1490890 - nsFlexContainerFrame::MarkIntrinsicISizesDirty is too aggressive

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

(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.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (benchmark which logs to console)

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.

Comment 2

[Daniel Holbert [:dholbert]]

(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.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: WIP

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.

Comment 4

[Daniel Holbert [:dholbert]]

The "WIP" patch drops testcase 1's 2nd measurement (the one I'm focusing on here) to the 5-6ms range, rather than ~260ms+.

Comment 5

[Daniel Holbert [:dholbert]]

(cloning qf status from bug 1377253 which this is a helper for)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1490890: Make flex item cached-measurement invalidation more targeted. r?emilio

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

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 8

[Daniel Holbert [:dholbert]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cda1e81d1293fc05b9db3afeb608442328c0ead

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7845d51b6f3c
Make flex item cached-measurement invalidation more targeted. r=emilio

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7845d51b6f3c