1495144 - Allow FrameConstructionData to be generated dynamically

Status: NEW

(Core :: Layout, enhancement, P3)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Currently, the FrameConstructionData is designed statically, which makes it hard to computed at runtime if we want to tweak it based on different CSS properties.

Take [1] for example, two "display: table-caption" and "overflow" result in a 2x2 table look-up for a FrameConstructionData. This doesn't scale if we want to support multi-keyword "display" property (bug 1038294).

Perhaps it will be good to allow FrameConstructionData to be generated dynamically. I'm not sure the performance impact on this, though.

Any thoughts?

[1] https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/layout/base/nsCSSFrameConstructor.cpp#4502-4547

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1495144 - Allow FrameConstructionData to be generated dynamically.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attached my proof of concept in comment 1.

Comment 3

[Mats Palmgren (inactive)]

The general idea seems good to me, assuming there's no perf regression.
We probably want to pack FrameConstructionData a little better if we
copy it around likes this.  Did you measure the before/after times of
frame construction on the single-page HTML spec page for example?

Comment 4

[Boris Zbarsky [:bzbarsky]]

For something like bug 1038294 we may actually want to refactor to have two sets of data: one for the outer bit and one for the inner.  Then again, it also feels like every frame should sort of become two objects: one that  participates in the parent and one that manages the children...

That aside, one option is to store a boolean in FrameConstructionData that indicates whether it's heap-allocated or not, have most of them static, and use heap-allocation in the rare cases that need them.  We'd need to make sure to not leak in those cases, which is admittely easier with the "copy it by value" approach.

FrameConstructionData is already packed pretty well, by the way.  Or at least I couldn't figure out a better way to pack it back when I created it.  :(  Maybe we can move the anon box pseudo into the flags if we replace it with an index into the atom array, but that's about it.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, I think using FrameConstructionData by value with an assertion that it remains smaller than a few words should be fine I think.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Measure ConstructDocElementFrame time.

Re comment 3:
I quickly measured the accumulated time of ConstructDocElementFrame() by opening a local single-page HTML spec. I run "./mach run ~/Downloads/HTML\ Standard.html --disable-e10s", and look for the "accumulated time" after the page is loaded.

Opt build without my patch (5 runs): 0.150272, 0.151229, 0.145739, 0.152924, 0.148444 second
Average: 0.1497216 second

Opt build *with* my patch (5 runs): 0.150069, 0.148184, 0.152989, 0.144088, 0.154166 second
Average: 0.1498992 second

It seems generating FrameConstructionData dynamically doesn't regress performance much.