Avoid heap allocations (usually) in BidiLineData

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf?])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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/.
(Assignee)

Comment 1

2 years ago
Created attachment 8859532 [details] [diff] [review]
Use AutoTArray for the array members in BidiLineData, to reduce heap allocation costs
Attachment #8859532 - Flags: review?(dholbert)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Attachment #8859532 - Flags: review?(dholbert) → review+
(Assignee)

Comment 3

2 years ago
(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).
(Assignee)

Comment 4

2 years ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf3ee74eb318
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.