Skip to content

Conversation

@symbiogenesis
Copy link
Contributor

@symbiogenesis symbiogenesis commented Jan 21, 2024

Flex.Item was an IEnumerable but internally stored a collection of Children as a List.

It seemed much simpler to just inherit from a list and remove all of the IEnumerable implementation and related extension methods.

Added a benchmark to confirm the suspicion.

Before:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
LayoutLotsOfItemsWithWrap 5.038 ms 0.1739 ms 0.5074 ms 171.8750 125.0000 62.5000 1.56 MB
LayoutLotsOfItemsNoWrap 4.280 ms 0.0836 ms 0.1057 ms 171.8750 125.0000 70.3125 1.56 MB

After:

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
LayoutLotsOfItemsWithWrap 4.384 ms 0.0805 ms 0.1930 ms 171.8750 156.2500 85.9375 1.57 MB
LayoutLotsOfItemsNoWrap 4.331 ms 0.0845 ms 0.0829 ms 171.8750 156.2500 85.9375 1.57 MB

@symbiogenesis symbiogenesis requested a review from a team as a code owner January 21, 2024 07:14
@ghost ghost added the community ✨ Community Contribution label Jan 21, 2024
@ghost
Copy link

ghost commented Jan 21, 2024

Hey there @symbiogenesis! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@samhouts samhouts added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Jan 25, 2024
@rmarinho
Copy link
Member

rmarinho commented Mar 5, 2024

/azp MAUI-UITests-public

@azure-pipelines
Copy link

Command 'MAUI-UITests-public' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@rmarinho
Copy link
Member

rmarinho commented Mar 5, 2024

/azp run MAUI-UITests-public

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

}

public void Add(Item child)
public new void Add(Item child)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inherit from List and uses the new on base methods is dangerous, if at some part of the code Item is cast to List<Item>, just the base.Add will be executed (The same for InsertAt). I believe having this class inherit from IEnumerable is to avoid this pitfall.

Copy link
Contributor Author

@symbiogenesis symbiogenesis Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a fair point. Although, Flex.Item is an internal class and I don't see anywhere in the codebase that it is being cast to a list.

Probably some of the same benefits could be attained by just figuring out how to remove the FlexExtensions file and simply using the built-in list functions of the Children property directly.

Of course, the approach of this PR also simplifies the code a bit.

@rmarinho rmarinho merged commit 8569185 into dotnet:main Mar 18, 2024
@symbiogenesis symbiogenesis deleted the flex-perf branch March 18, 2024 21:31
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter community ✨ Community Contribution fixed-in-8.0.20 fixed-in-9.0.0-preview.3.10457

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants