Fix performance regression for EventsDispatching [chromium/src : main]
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
Nice! LGTM. Let's make sure to check back in on the regression bug plots after this lands, to see what impact it has there, ok?
optimization, performance was measured at an average of 317 runs/s.317? Looks more like 371 or so?
RuntimeEnabledFeatures::TargetInShadowDeterminedBeforeListenerEnabled()Can you do me a favor and extend the "can be removed" deadline to M147, given that we're still tweaking the behavior?
Related details
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 3 comments![]( "Open in Gerrit")
Thanks for the review!
I’ve addressed all the review comments, but during additional testing I noticed some unusual results, and I think it may require a bit more discussion.
It seems that performance varies depending on the PGO data. As evidence, when the flag is removed and the test is run on the main branch, performance improves.
However, around the time when [1] was merged, this CL resulted in a performance improvement, but on the main branch (with updated PGO data), it appears that this CL instead causes a performance regression.
Given this, it feels like we may not need to take any additional action here, and that simply removing the flag at the appropriate time should be sufficient.
Please feel free to share any other thoughts or suggestions.
Thanks!
[1] https://chromium-review.googlesource.com/c/chromium/src/+/6966287
optimization, performance was measured at an average of 317 runs/s.317? Looks more like 371 or so?
Oh! You’re right. This was my mistake.
Thanks for pointing it out
RuntimeEnabledFeatures::TargetInShadowDeterminedBeforeListenerEnabled()Can you do me a favor and extend the "can be removed" deadline to M147, given that we're still tweaking the behavior?
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Code-Review+1![]( "Open in Gerrit")
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Thanks for the LGTM!
However, there might be an issue like the one mentioned in [1].
Would it be okay to merge this as is?
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7260454/comments/d1700846_d5b91c93
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
Thanks for the review!
I’ve addressed all the review comments, but during additional testing I noticed some unusual results, and I think it may require a bit more discussion.
It seems that performance varies depending on the PGO data. As evidence, when the flag is removed and the test is run on the main branch, performance improves.
However, around the time when [1] was merged, this CL resulted in a performance improvement, but on the main branch (with updated PGO data), it appears that this CL instead causes a performance regression.
Given this, it feels like we may not need to take any additional action here, and that simply removing the flag at the appropriate time should be sufficient.Please feel free to share any other thoughts or suggestions.
Thanks![1] https://chromium-review.googlesource.com/c/chromium/src/+/6966287
I don't think there's a harm in landing this, since it should improve non-PGO performance also. I'm guessing you're correct, that PGO "notices" that the flag is always true, and therefore that the `Node* target_node` code isn't needed. PGO effectively probably does what this CL does explicitly. But I don't think there's harm in landing this, and I think we should.
Thanks for the LGTM!
However, there might be an issue like the one mentioned in [1].
Would it be okay to merge this as is?[1] https://chromium-review.googlesource.com/c/chromium/src/+/7260454/comments/d1700846_d5b91c93
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Mason FreedThanks for the review!
I’ve addressed all the review comments, but during additional testing I noticed some unusual results, and I think it may require a bit more discussion.
It seems that performance varies depending on the PGO data. As evidence, when the flag is removed and the test is run on the main branch, performance improves.
However, around the time when [1] was merged, this CL resulted in a performance improvement, but on the main branch (with updated PGO data), it appears that this CL instead causes a performance regression.
Given this, it feels like we may not need to take any additional action here, and that simply removing the flag at the appropriate time should be sufficient.Please feel free to share any other thoughts or suggestions.
Thanks![1] https://chromium-review.googlesource.com/c/chromium/src/+/6966287
I don't think there's a harm in landing this, since it should improve non-PGO performance also. I'm guessing you're correct, that PGO "notices" that the flag is always true, and therefore that the `Node* target_node` code isn't needed. PGO effectively probably does what this CL does explicitly. But I don't think there's harm in landing this, and I think we should.
Thanks for the feedback! I’ll go ahead and merge it.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim added 1 comment![]( "Open in Gerrit")
Please take a look. Thank you!
Related details
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted Code-Review+1![]( "Open in Gerrit")
- YeongHan Kim
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
YeongHan Kim (Gerrit)
YeongHan Kim voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix performance regression for EventsDispatching
Patch [1] introduced a performance regression. The cause might be
related to a profile-guided optimization issue[2].
The reason is that the asymmetric use of target_node, only when the
flag is false, may hinder effective PGO optimization. After the code
optimization, performance was measured at an average of 371 runs/s.
Before After
1st 342 runs/s 372 runs/s
2nd 339 runs/s 370 runs/s
3rd 340 runs/s 371 runs/s
4th 339 runs/s 373 runs/s
5th 341 runs/s 369 runs/s
avg 340 runs/s 371 runs/s
[1] https://chromium-review.googlesource.com/c/chromium/src/+/6966287
[2] https://en.wikipedia.org/wiki/Profile-guided_optimization
- M third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
Code-Review: +1 by Kentaro Hara, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |