Closed Bug 1384669 Opened 7 years ago Closed 7 years ago

Avoid refcount churn in BidiParagraphData::ResetData

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Performance Impact high
Tracking Status
firefox56 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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?
Blocks: 1373816
Flags: needinfo?(jfkthame)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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+
Flags: needinfo?(jfkthame)
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
https://hg.mozilla.org/mozilla-central/rev/702f9dbca19f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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]
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.