Closed
Bug 1490890
Opened 6 years ago
Closed 6 years ago
nsFlexContainerFrame::MarkIntrinsicISizesDirty is too aggressive
Categories
(Core :: Layout: Flexbox, enhancement, P3)
Core
Layout: Flexbox
Tracking
()
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
(Spinning this off for part of bug 1377253 that I want to land on its own.) Right now nsFlexContainerFrame::MarkIntrinsicISizesDirty clears the CachedFlexMeasuringReflow() frame-property on all flex items. This is unnecessary in most cases. I think the flex items' CachedFlexMeasuringReflow are still valid most of the time, though I need to think harder about whether there are scenarios where we do need to discard the CachedFlexMeasuringReflow data. (One case where we definitely need to discard it: when the item itself has its intrinsic sizes marked as dirty. We can handle that in the generic nsFrame::MarkIntrinsicISizesDirty method, similar to how we invalidate for XUL boxes there.)
Assignee | ||
Comment 1•6 years ago
|
||
Here's a testcase, based on the codemirror scenario from bug 1485084. This testcase logs two measurements. * For the first one ("Layout duration for append:"), we happen to be faster than Chrome right now. * For the second one, "Layout duration for tweak in other flex item"), we currently report ~260ms, whereas Chrome reports 0ms or 1ms. This bug is about getting us to be closer to the 0ms / 1ms range for this second measurement.
Assignee | ||
Updated•6 years ago
|
Attachment #9008611 -
Attachment description: testcase 1 → testcase 1 (benchmark which logs to console)
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #1) > Here's a testcase, based on the codemirror scenario from bug 1485084. Specifically: the second measurement in this testcase (the measurement I'm focusing on improving here) is basically measuring the keypress reaction time in the CodeMirror "typing in web console" scenario.
Assignee | ||
Comment 3•6 years ago
|
||
Here's what I've got at the moment, though I need to convince myself that it's correct (and maybe add some special cases if it's not). This is a subset of the WIP patch that I'd posted for feedback in bug 1485084 comment 5, btw. I'm pretty sure this is the key part of that patch that helps for this CodeMirror / web-console-interactivity use case.
Assignee: nobody → dholbert
Assignee | ||
Comment 4•6 years ago
|
||
The "WIP" patch drops testcase 1's 2nd measurement (the one I'm focusing on here) to the 5-6ms range, rather than ~260ms+.
Assignee | ||
Comment 5•6 years ago
|
||
(cloning qf status from bug 1377253 which this is a helper for)
Whiteboard: [qf:p1:f64]
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Updated•6 years ago
|
Component: Layout → Layout: Flexbox
Assignee | ||
Comment 6•6 years ago
|
||
Right now, when a flex item's intrinsic size is invalidated, we clear cached flex measurements on all of its sibling flex items (indirectly, by virtue of invalidating the flex container's intrinsic sizes, which does the dirty work of clearing the measurements). This is excessive. We do need to clear the measurements on any flex item whose intrinsic sizes are invalidated, but we don't need to clear them on other flex items whose intrinsic sizes are still valid. So: this patch changes our implementation accordingly. This patch isn't expected to change behavior - it should just be an optimization.
Comment 7•6 years ago
|
||
Comment on attachment 9009287 [details] Bug 1490890: Make flex item cached-measurement invalidation more targeted. r?emilio Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #9009287 -
Flags: review+
Assignee | ||
Comment 8•6 years ago
|
||
try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cda1e81d1293fc05b9db3afeb608442328c0ead
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7845d51b6f3c Make flex item cached-measurement invalidation more targeted. r=emilio
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7845d51b6f3c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64]
You need to log in
before you can comment on or make changes to this bug.
Description
•