Skip to content
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

Perf #307

Merged
merged 6 commits into from
Sep 9, 2016
Merged

Perf #307

merged 6 commits into from
Sep 9, 2016

Conversation

blasten
Copy link
Contributor

@blasten blasten commented Sep 1, 2016

Ref: #301

Timeline for first paint

Current (waiting 16ms, job=as many nodes as they fit on the list's viewport):

before

After (waiting for requestIdleCallback, ~50ms job):

after-ricb

Conclusion:

Looking at frames for the offscreen items (the ones after the first frame on the screenshot), we can conclude that this PR does make the app more responsive by yielding control back to the main. It does that by keeping track of the cost of stamping a single template and then estimating how many instances of that template could be grouped into a block of ~50ms as recommended by RAIL

cc @justinfagnani

Polymer.dom.flush();
// Items should have been rendered prior scrolling to an index.
if (!this._itemsRendered) {
Copy link
Contributor

@frankiefu frankiefu Sep 2, 2016

Choose a reason for hiding this comment

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

Remove _itemsRendered from the properties object.

@blasten
Copy link
Contributor Author

blasten commented Sep 2, 2016

@tgsergeant would you mind trying this out?

@tgsergeant
Copy link

Cool! This is a definite improvement. Here's a trace from chrome://history with this patch applied. It splits the stamping into 5 sections, with around 15-20fps. With devtools CPU throttling turned on, it correctly splits into more sections and maintains a similar frame rate.

One thing to consider is that there can be an extra 10-20ms worth of work after the iron-list yields to update layers and paint and so on. This adds on to the 50ms allocated for stamping items, and means that frames can last longer than intended.

j642d2xqpqn

@tgsergeant
Copy link

However, this change does seem to have caused the stamping of the first page to get slower. I'm not sure what is causing the difference.

Old:
old trace

New:
new trace

@blasten
Copy link
Contributor Author

blasten commented Sep 5, 2016

@tgsergeant could you export and share with me the timeline data for both the old and new version? I think I might know why.

@blasten
Copy link
Contributor Author

blasten commented Sep 6, 2016

@tgsergeant this commit should fix the issue. Would you mind giving it a try?

Also, fyi: PolymerElements/paper-checkbox#149. You should be able to use paper-checkbox-light when it's ready.

@tgsergeant
Copy link

👍, that looks more like it. Thanks!

(and yes, we're eagerly awaiting paper-checkbox-light)

@danbeam
Copy link
Contributor

danbeam commented Sep 9, 2016

so what's the status on this? it'd be really helpful for Chrome's history UI

@blasten
Copy link
Contributor Author

blasten commented Sep 9, 2016

@danbeam I'm testing a few things and then it should be ready to merge.

@frankiefu
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants