1354218 - Consider storing FrameBidiData directly in nsIFrame instead of as a frame property

Status: RESOLVED WONTFIX

(Core :: Layout, enhancement)

Description

[Jonathan Kew [:jfkthame]]

When bidi content is present, almost every text frame (and some others) ends up getting a FrameBidiData attached, involving the hashtable-lookup dance to access the FramePropertyTable. (See bug 1348469 comment 6.)

Now, FrameBidiData is only a 3-byte struct. And as it happens, there are 3 bytes of padding in nsIFrame, next to the (single-byte) mWritingMode member.

This means that we can store the FrameBidiData directly in nsIFrame, and thus eliminate many thousands of hashtable accesses for a document like the example analyzed in bug 1348469, without adding to the frame memory footprint at all. (In fact, this will reduce overall memory use by avoiding the creation of all those hashtable entries.)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Store FrameBidiData in nsIFrame itself, instead of using a frame property

AFAICS, this preserves existing behavior, and just eliminates the use of the hashtable for this data. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=24e66efc82c14486c1ac5724c6844f8626a10b98.

Comment 2

[Jonathan Kew [:jfkthame]]

:mats - review ping - do you think this is a worthwhile optimization?

Comment 3

[Jonathan Kew [:jfkthame]]

Incidentally, I tried comparing profiles without and with this patch, and on my MBPro it appears to shave something like 80ms off the initial load/reflow of https://html.spec.whatwg.org/. So not huge, but nice to have.

Comment 4

[Mats Palmgren (inactive)]

I'll try to get to this review tomorrow.  Sorry, for the delay.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8855421 [details] [diff] [review]
Store FrameBidiData in nsIFrame itself, instead of using a frame property

Fwiw, I did some more measurements on the single-page HTML spec.
There are only 13 text nodes on the whole page that trigger
aDocument->SetBidiEnabled().  We do 56567 calls to 
nsBidiPresUtils::Resolve, which results in 160960 frames
having a BidiDataProperty.

This points to a problem in the architecture / algorithms of bidi-
resolution, rather than what is optimized here.  I saw that you're
addressing some of that in bug 1358275 which is great.  I think
we can do more here.  For example, we essentially do a separate
pass over the frame tree in ResolveBidi which is expensive.
I looked briefly at the Chromium code and it looked to me like
they have integrated bidi-resolution into their layout pass.
Can we also do this during Reflow?

Anyway, I'm not convinced we should do this optimization (not yet anyway)
for the following reasons:
1. pages that triggers bidi resolution are generally very rare
   (granted though, for a minority of users it's the common case)
   Do we have data on this?  (I'd guess the number of pages that
   trigger bidi-resolution to be well below 1%.)
2. the improvement from this patch seems negligible even for a huge page
   like the single-page HTML spec
3. using 3 bytes on the base class of all frames is relatively expensive,
   given it's rarely needed and give a negligible improvement

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #5)
> Anyway, I'm not convinced we should do this optimization (not yet anyway)
> for the following reasons:
> 1. pages that triggers bidi resolution are generally very rare
>    (granted though, for a minority of users it's the common case)
>    Do we have data on this?  (I'd guess the number of pages that
>    trigger bidi-resolution to be well below 1%.)

My concern here is that although the overall proportion of pages may be quite low, there are high-profile examples where bidi-resolution is activated (for the entire page, even though its "real" content may be purely LTR) due to the presence of an isolated fragment of bidi text; a typical example would be the Wikipedia "languages" sidebar.

However, it's possible that the approach in bug 1358275 will reduce the impact of cases like that. I'm intending to do some more instrumentation to see how that goes. So for now, I'm fine with leaving this aside.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #5)
> Comment on attachment 8855421 [details] [diff] [review]
> Store FrameBidiData in nsIFrame itself, instead of using a frame property
> 
> Fwiw, I did some more measurements on the single-page HTML spec.
> There are only 13 text nodes on the whole page that trigger
> aDocument->SetBidiEnabled().  We do 56567 calls to 
> nsBidiPresUtils::Resolve, which results in 160960 frames
> having a BidiDataProperty.

FTR, I just did a little testing and found that the patches in bug 1358275 will reduce the number of frames that get a BidiDataProperty here to well under 4000.

I also considered the idea of encoding the bidi data into a single byte (instead of using all 3 of the currently-spare bytes) stored directly in nsIFrame for common cases (i.e. the baseLevel and embeddingLevel values will rarely exceed about 3 or 4, and precedingControl is rarely needed), and just falling back to a separate property in the pathological complex cases with deep embedding, etc. But I'm not convinced it would be worth the added code complexity.

So at this point I'm going to WONTFIX this.