1357694 - Avoid heap allocations (usually) in BidiLineData

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Jonathan Kew [:jfkthame]]

The BidiLineData struct contains several nsTArray members, which end up doing heap allocations. We can avoid this (for all but the most pathologically complex cases) by making these AutoTArrays instead. BidiLineData is a short-lived stack-allocated struct, and AFAICT is not used recursively, so the increase in the struct's size is relatively unimportant compared to avoiding several heap allocations every time it's used.

In testing on my MBPro, this shaves somewhere around 100ms off the time for a window-resize reflow of https://html.spec.whatwg.org/.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Use AutoTArray for the array members in BidiLineData, to reduce heap allocation costs

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8859532 [details] [diff] [review]
Use AutoTArray for the array members in BidiLineData, to reduce heap allocation costs

Review of attachment 8859532 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits:

::: layout/base/nsBidiPresUtils.cpp
@@ +371,5 @@
>  };
>  
>  struct BidiLineData {
> +  AutoTArray<nsIFrame*, 16> mLogicalFrames;
> +  AutoTArray<nsIFrame*, 16> mVisualFrames;

Please declare the struct as MOZ_STACK_CLASS (and add an include for mozilla/Attributes.h to provide that), to enforce that these AutoTArrays are really on the stack.

@@ +418,5 @@
>  
>      // Strip virtual frames
>      if (hasVirtualControls) {
>        auto originalCount = mLogicalFrames.Length();
> +      AutoTArray<int32_t, 16> realFrameMap;

If you can (and I think you can), we should still pass in |originalCount| to this constructor, like the old code does.

When originalCount is under 16, it won't matter, & that's fine.  But for cases when |originalCount| is larger than 16 (and especially when it's significantly larger), this will let us preallocate the correct amount of heap memory right away, rather than spinning our wheels with the default nsTArray doubling behavior.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Comment on attachment 8859532 [details] [diff] [review]
> Use AutoTArray for the array members in BidiLineData, to reduce heap
> allocation costs
> 
> Review of attachment 8859532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits:
> 
> ::: layout/base/nsBidiPresUtils.cpp
> @@ +371,5 @@
> >  };
> >  
> >  struct BidiLineData {
> > +  AutoTArray<nsIFrame*, 16> mLogicalFrames;
> > +  AutoTArray<nsIFrame*, 16> mVisualFrames;
> 
> Please declare the struct as MOZ_STACK_CLASS (and add an include for
> mozilla/Attributes.h to provide that), to enforce that these AutoTArrays are
> really on the stack.

Will do.

> @@ +418,5 @@
> >  
> >      // Strip virtual frames
> >      if (hasVirtualControls) {
> >        auto originalCount = mLogicalFrames.Length();
> > +      AutoTArray<int32_t, 16> realFrameMap;
> 
> If you can (and I think you can), we should still pass in |originalCount| to
> this constructor, like the old code does.

That doesn't build; AutoTArray doesn't have a constructor that takes an initial capacity. I suppose we could call SetCapacity(originalCount) on it immediately after construction, so that if it does need enlarging, we'll only do one allocation rather than potentially several steps. I'll add that (though I suspect it'll be so rare as to be insignificant in practice).

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3ee74eb31893e0f0bbcb3f6ffdc329043b3c70
Bug 1357694 - Use AutoTArray for the array members in BidiLineData, to reduce heap allocation costs. r=dholbert

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf3ee74eb318