Skip to content

Reduce closure allocation in RenderObject.cleanRelayoutBoundary #51439

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.

Sign up for GitHub

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

Jump to bottom
Merged
ferhatb merged 1 commit into from
Feb 26, 2020
Merged

Reduce closure allocation in RenderObject.cleanRelayoutBoundary #51439

ferhatb merged 1 commit into from
Feb 26, 2020

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Feb 25, 2020

Description

Changes visitChildren closure allocation to use static method instead.

Related Issues

Discovered while profiling web EAP client project.

Tests

Covered by gallery benchmark.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

@ferhatb ferhatb requested a review from yjbanov February 25, 2020 22:16
@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 25, 2020
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Feb 25, 2020

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@ferhatb ferhatb requested a review from goderbauer February 26, 2020 00:23
goderbauer
goderbauer approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@yjbanov
Copy link
Contributor

yjbanov commented Feb 26, 2020

/cc @vsmenon @sigmundch could this also be improved in the compiler?

@ferhatb ferhatb merged commit 1a79592 into flutter:master Feb 26, 2020
@vsmenon
Copy link

vsmenon commented Feb 26, 2020

@rakudrama - on optimizability

Non-trivial if compiler cannot identify whether visitChildren, etc rely on identity of the closure.

@rakudrama
Copy link

rakudrama commented Feb 27, 2020

This has been discussed at: dart-lang/sdk#26932
I'm prepared to break identity in optimizations - dart2js already GVNs repeated tear-offs which also have identity.

Since the language has evolved to have much richer static constraints (e.g. definite assignment is quite involved), I would advocate for a static definition of free variables and have the CFE hoist the functions.

@rakudrama
Copy link

rakudrama commented Feb 27, 2020

@leafpetersen How about updating the language spec so that closures with no free variables are more like static or top-level methods?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yjbanov yjbanov yjbanov approved these changes

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ferhatb @fluttergithubbot @yjbanov @vsmenon @rakudrama @goderbauer @googlebot