nsFlexContainerFrame::MarkIntrinsicISizesDirty is too aggressive

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [qf:p1:f64])

Attachments

(3 attachments)

(Assignee)

Description

6 months ago
(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 months 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 months ago
Attachment #9008611 - Attachment description: testcase 1 → testcase 1 (benchmark which logs to console)
(Assignee)

Comment 2

6 months 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 months ago
Posted patch WIPSplinter Review
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 months 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 months ago
(cloning qf status from bug 1377253 which this is a helper for)
Whiteboard: [qf:p1:f64]
Blocks: 1473805
Priority: -- → P3
(Assignee)

Updated

6 months ago
Component: Layout → Layout: Flexbox
(Assignee)

Comment 6

6 months 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 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+

Comment 9

6 months ago
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 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7845d51b6f3c
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

6 months ago
Blocks: 1482798
Blocks: 1408190
Blocks: 1410464
Depends on: 1493645

Updated

5 months ago
Blocks: 1503173
(Assignee)

Updated

5 months ago
Blocks: 1505344
You need to log in before you can comment on or make changes to this bug.