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

core: enable async stacks #5504

Merged
merged 4 commits into from
Apr 18, 2019
Merged

core: enable async stacks #5504

merged 4 commits into from
Apr 18, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jun 14, 2018

Summary
Initiator information can fallback to parser in cases when elements were added inside a microtask/Promise chain. This hurts the fidelity of the lantern graph.

Even though async stacks are enabled by default in stable Chrome now in v8 (https://v8.dev/blog/v8-release-73), it doesn't apply to the initiator information we get from Chrome which still requires explicitly setting the async stack depth.

I would love to get this in for 5.0 as it enables a lot of performance accuracy improvements and better attribution for bootup-time/third-party-web

Related Issues/PRs
fixes #5494
closes #3375

@patrickhulce
Copy link
Collaborator Author

this is essentially waiting on #6152 to validate the impact of this

@patrickhulce
Copy link
Collaborator Author

DZL, do a barrel roll!

@devtools-bot
Copy link

DZL is done! Go check it out http://lh-dzl-5504.surge.sh

@brendankenny
Copy link
Member

is it weird none of the yarn test-lantern numbers changed?

@patrickhulce
Copy link
Collaborator Author

@brendankenny no that's expected. Those are based on static traces/devtoolslogs, and this changes the devtoolslog for the full effect.

@patrickhulce
Copy link
Collaborator Author

The performance metrics charts look pretty good from DZL on this, so I think we should proceed. Our primary concern would be if we saw substantial increases in TTI and that's not the case. The fact that zero-cost async stacks should already be enabled and this is just propagating that information over to network record initiators makes this a little less concerning as well.

Review away 😃

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

  • DZL looks unchanged, so Debugger isn't the boogeyman we thought, but is there a good example somewhere of this helping things? :)

  • smoke test? Might be too hard to get a decent and testable change in metrics, but now with artifact testing we could at least assert network records if that's not the worst idea you've ever heard :)

lighthouse-core/computed/page-dependency-graph.js Outdated Show resolved Hide resolved
lighthouse-core/gather/driver.js Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Apr 17, 2019

@brendankenny if we had jest for smoketests (#8190) it'd be a piece of cake, but right now it's extremely non-intuitive test to understand and the failure mode would be very difficult to debug. As this is mostly an enablement thing and isn't meant to suddenly have a big impact on its own (we need to add the logic to start taking advantage of async stacks in the places I mentioned), I think it makes sense to land without it.

I have the smoketest ready to go locally post #8190 or something like it.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

@paulirish should probably also take a gander

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