Avoid refcount churn in BidiParagraphData::ResetData

RESOLVED FIXED in Firefox 56

Status

()

Core
Layout: Text
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

BidiParagraphData::ResetData shows up in this Facebook profile (just 1 sample, but this is on reference hardware, so samples are sparse).

Specifically, we get a sample with this backtrace (listed here from outermost to innermost function):

nsBidiPresUtils::Resolve(nsBlockFrame *)
nsBidiPresUtils::TraverseFrames(nsBlockInFlowLineIterator *,nsIFrame *,BidiParagraphData *)
BidiParagraphData::ResetData()
PLDHashTable::ClearAndPrepareForLength(unsigned int)
PLDHashTable::~PLDHashTable()
nsBaseHashtableET<nsHashKeyDisallowMemmove<nsURIHashKey>,JS::Heap<JSScript *> >::`scalar deleting destructor'(unsigned int)
nsCOMPtr_base::~nsCOMPtr_base()
mozilla::dom::HTMLBodyElement::Release()

https://perf-html.io/public/371f3f0e2de5840fdfea78d46e35967044fe1e45/calltree/?hiddenThreads=&invertCallstack&search=ResetData&thread=6&threadOrder=0-2-3-4-6-1-5

So -- why are we Release'ing a nsCOMPtr for the <body> element inside of this Bidi code? I think it's because BidiParagraphData has this member-var:
  nsDataHashtable<nsISupportsHashKey, int32_t> mContentToFrameIndex;
...and nsISupportsHashKey has a nsCOMPtr:
https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/xpcom/ds/nsHashKeys.h#292,319

(Side note: I think the mention of nsURIHashKey/JSScript are bogus here, in the backtrace's hashtable template -- those must fall out of a compiler optimization about making the two templated classes share some implementation details, or something.)

If this runs during layout (when the DOM is static), I suspect we could really just be using a raw nsIContent pointer in our hash key.  Note that BidiParagraphData is annotated as MOZ_STACK_CLASS, so it can't persist beyond the scope of a single reflow call, so I think this should be fine.

jfkthame, does this seem sane?

Updated

10 months ago
Flags: needinfo?(jfkthame)
Comment hidden (mozreview-request)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED

Comment 2

10 months ago
mozreview-review
Comment on attachment 8890519 [details]
Bug 1384669: Reduce refcount churn in BidiParagraphData by using non-refcounted pointer hash keys.

https://reviewboard.mozilla.org/r/161654/#review166972

Makes sense to me.
Attachment #8890519 - Flags: review?(jfkthame) → review+

Updated

10 months ago
Flags: needinfo?(jfkthame)

Comment 3

10 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/702f9dbca19f
Reduce refcount churn in BidiParagraphData by using non-refcounted pointer hash keys. r=jfkthame

Comment 4

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/702f9dbca19f
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Calling this qf:p1, since it was for (small) perf issue identified from a real-wold facebook gecko-profile.
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.