Skip to content

Consider copying MultiChildRenderObjectWidget.children #48321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rakudrama opened this issue Jan 7, 2020 · 10 comments
Open

Consider copying MultiChildRenderObjectWidget.children #48321

rakudrama opened this issue Jan 7, 2020 · 10 comments
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) c: proposal A detailed proposal for a change to Flutter framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list perf: speed Performance issues related to (mostly rendering) speed team-framework Owned by Framework team triaged-framework Triaged by Framework team

Comments

@rakudrama
Copy link

One of the problems with the Flutter model is that developers can accidentally or intentionally mutate the Widget tree via mutating MultiChildRenderObjectWidget.children. See dart-lang/sdk#27755.

I think there might be a performance reason to copy the children. It depends on how often the framework walks the children.

List is an interface that has many implementations.
The VM has _List for fixed-length lists and _GrowableList for 'regular' lists, and another implementation (I don't recall the name) for unmodifiable and const Lists.

In a small Flutter app, all the children are likely from List literals or to calls to .toList(), so the children field is likely to have just two or three implementation classes. The JITing VM and AOT try to discover this, and the three implementation classes are carefully crafted to be as alike as possible. This makes it possible to compile children[i] to some indexing code.

It is possible that in a large Flutter app that there are other implementations of List that are passed in as the children, meaning it is no longer possible to compile children[i] to indexing code since some of the implementations have custom indexers and .length getters that must be called.

My hypothesis is that as the inner loop in the framework becomes more polymorphic, the performance of this code shared by all multi-child widgets degrades.

One could test this hypothesis by taking a benchmark that is where walking the children is 'hot'. I expect the benchmark would walk the 'the same' a large Widget tree over and over, with no actual appearance change. Alter one of the constructors to pass in an UnmodifiableListView([....]) instead of [....]. This will cause the walking of children[i] to become less efficient. Does this show up in the benchmark?

If you decide to protect the performance by copying (which ensures there is only one kind of List), I would suggest copying with List<Widget>.unmodifiable(input).
A small fixed-length or unmodifiable list can be is half the size of a growable list or less. If the growable input is no longer referenced, it can be GC-ed. The copy will do more allocation, but have a smaller size post-GC. This may be a reasonable tradeoff.

Making the children unmodifiable will prevent users from modifying the trees they create which will help them avoid a class of bugs.
It will break the problematic pattern of widget.children.add(w).
Even if the above hypothesis does not pan out, copying the list to an unmodifiable list in debug mode would help developers avoid bugs.

@Hixie Hixie added perf: speed Performance issues related to (mostly rendering) speed c: performance Relates to speed or footprint issues (see "perf:" labels) labels Jan 7, 2020
@Hixie Hixie added this to the Goals milestone Jan 7, 2020
@Hixie Hixie added the framework flutter/packages/flutter repository. See also f: labels. label Jan 7, 2020
@Hixie

This comment has been minimized.

@Hixie

This comment has been minimized.

@icatalud

This comment has been minimized.

@Hixie
Copy link
Contributor

Hixie commented Jan 10, 2020

@icatalud The experiment is to determine whether copying child lists will improve or hurt performance. We had previously assumed it would hurt performance (because memory copying is expensive) but @rakudrama observed that it's conceivable that it would have other benefits that would actually outweigh the copying, and we should measure it to see.

@icatalud

This comment has been minimized.

@icatalud

This comment has been minimized.

@Hixie

This comment has been minimized.

@icatalud

This comment has been minimized.

@Hixie

This comment has been minimized.

@kf6gpe kf6gpe added the P2 Important issues not at the top of the work list label May 29, 2020
@Hixie Hixie removed this from the None. milestone Aug 17, 2020
@pedromassangocode
Copy link

Labelling proposal!

@pedromassangocode pedromassangocode added the c: proposal A detailed proposal for a change to Flutter label Sep 24, 2020
@flutter-triage-bot flutter-triage-bot bot added team-framework Owned by Framework team triaged-framework Triaged by Framework team labels Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) c: proposal A detailed proposal for a change to Flutter framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list perf: speed Performance issues related to (mostly rendering) speed team-framework Owned by Framework team triaged-framework Triaged by Framework team
Projects
None yet
Development

No branches or pull requests

5 participants