📍 Job win-11-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15981c59310000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Set Ready For Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job mac-m1_mini_2020-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17706772310000
📍 Job android-pixel4-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15348f06310000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/56394.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
| Code-Review | +0 |
Hi Daniil, sorry, I +1'd because I thought the pinpoints were good, but I see they are not.
The perf test is third_party/blink/perf_tests/layout/hittest-block-children.html. Can you open this in a regular profiler and determine where it is slow? I took a quick look with this patch (https://screenshot.googleplex.com/AzLACuh6F25c2d9) and I don't see anything obviously wrong, so I think you will need to compare profiles with patch vs without patch. One possibility is that HitTestChildren is called more, maybe due to skip_children changes in LayoutBox::NodeAtPoint?
#include "third_party/blink/renderer/core/layout/layout_box.h"Why this change?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job win-11-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11b39181310000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job mac-m1_mini_2020-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17db973d310000
📍 Job android-pixel4-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/179979c9310000
📍 Job mac-m1_mini_2020-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/153a7a43310000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
if (HasHitTestableOverflow()) {This looks a little suspicious. With hit testable overflow and clip, the old code would do:
overflow_box = VisualOverflowRect();
overflow_box.Intersect(OverflowClipRect(PhysicalOffset()));
overflow_box.Unite(PhysicalBorderBoxRect());
The new code does:
overflow_box = PhysicalBorderBoxRect();
PhysicalRect visual_overflow = VisualOverflowRect();
visual_overflow.Intersect(OverflowClipRect(PhysicalOffset()));
overflow_box.Unite(visual_overflow);
I didn't verify this is different, just seems like it may be.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job android-pixel4-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/10926db5310000
| Commit-Queue | +1 |
#include "third_party/blink/renderer/core/layout/layout_box.h"Why this change?
ah, git cl format keeps moving that thing for me 😊
if (HasHitTestableOverflow()) {This looks a little suspicious. With hit testable overflow and clip, the old code would do:
overflow_box = VisualOverflowRect();
overflow_box.Intersect(OverflowClipRect(PhysicalOffset()));
overflow_box.Unite(PhysicalBorderBoxRect());The new code does:
overflow_box = PhysicalBorderBoxRect();
PhysicalRect visual_overflow = VisualOverflowRect();
visual_overflow.Intersect(OverflowClipRect(PhysicalOffset()));
overflow_box.Unite(visual_overflow);I didn't verify this is different, just seems like it may be.
updated back to the correct way now
This is also incorrect if fragmented.
yes, but we decided to start with a simple case and improve afterwards
Where is this specified?
it's not yet specced, but we agreed with Morten and Noam that having overscroll for border-shape would make the most sense here.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job mac-m1_mini_2020-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1562b67c710000
📍 Job android-pixel4-perf/blink_perf.layout complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/120288b8710000
Daniil SakhapovWhere is this specified?
it's not yet specced, but we agreed with Morten and Noam that having overscroll for border-shape would make the most sense here.
So I'm not sure it does, e.g. we don't for other decoration like things such as box-shadow.
if (StyleRef().HasBorderShape()) {Theory: is accessing this StyleRef() causing us to pull a big object into the cache? Previously, this function didn't need to use StyleRef, but now it does. Can we use a bitfield check like `HasScrollableOverflow` to avoid accessing data on styleref in the common case?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |