222436 - GetBidiEnable call is very expensive

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ivan Eisen]

I traced 2 testcases and found that GetBidiEnable is costing 2.45% of the
total time in one and 2.11% of the total time in the second.  
The first testcase is a document containing 2000 hotlinks the second is a simple 
large test document.  The test was run using Moz 1.4 under Linux.

See trace segments of 2 tests in attachments.

Comment 1

[Ivan Eisen]

Attachment: Shows who calls GetBidiEnable

Comment 2

[Ivan Eisen]

Note, the second test was a large text document.  

The BidiEnable switch
should be cached to prevent so many calls to get it.  

Comment 3

[Boris Zbarsky [:bzbarsky]]

hmm... Does changing nsPresContext::GetBidiEnabled from doing

1562   if (mShell) {
1563     nsCOMPtr<nsIDocument> doc;
1564     mShell->GetDocument(getter_AddRefs(doc) );
1565     NS_ASSERTION(doc, "PresShell has no document in
nsPresContext::GetBidiEnabled");
1566     if (doc) {
1567       doc->GetBidiEnabled(aBidiEnabled);
1568     }

to doing

mShell->GetDocument()->GetBidiEnabled(aBidiEnabled);

speed things up any?  That could be further inlined once bug 222134 lands.

Comment 4

[Sam Emrick]

The bulk of these calls seem to be the result of nsHTMLReflowState 
constructor,and varients like nsBlockReflowState. The deCOM work is 
goodness, in general, and may be enough. It may be worthwhile to look 
at where bulk of reflowstate constructors are generated, and try to 
reduce their use, at least reduce their construction/destruction. 
Potential may be larger than just the 2.5% GetBidiEnable that the 
constructors are generating.

Comment 5

[Boris Zbarsky [:bzbarsky]]

bryner, are there any cases when an nsPresContext will have an mShell but that
won't have a document?

Comment 6

[Timothy Nikkel (:tnikkel)]

Is this still a hot spot? Shouldn't be too hard to save a mBidiEnabled on the PresContext and have the document tell the PresContext to update it when the document's mBidiEnabled changes.

Comment 7

[Timothy Nikkel (:tnikkel)]

I hacked up a quick patch to do that. I measured the time from setting location.href to a local copy of the testcase in bug 542877, comment 0 until the load event fired. I did about 20 runs each, times ranged from about 4000 ms to 5000 ms. The average with the patch was about 100ms or so lower than without. I don't know if this is statistically significant due to variation in times.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just inlining GetBidiInternal() and making it "return Document()->GetBidiEnabled()"? That's simpler than the current code and I don't believe we could make it significantly faster even if we cached a boolean in the prescontext.

Comment 9

[Timothy Nikkel (:tnikkel)]

Doing more runs and producing a box plot showed clearly that there was no improvement (either with comment 7 or comment 8). So before doing anything else I need a testcase and/or a profile showing nsPresContext::BidiEnabled actually has an impact.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we do what I suggested in comment #8, the code is simpler as well as potentially faster, so I recommend we just do that.

Comment 11

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Ok, let's do that.

Comment 12

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/3a27158cbdd6

Comment 13

[Timothy Nikkel (:tnikkel)]

Backed out because something in the push caused tsvg_opacity regression.
http://hg.mozilla.org/mozilla-central/rev/df3a1a39837f
http://hg.mozilla.org/mozilla-central/rev/d5da99049f49

Comment 14

[Timothy Nikkel (:tnikkel)]

I determined that the regression was caused by bug 553359.

Landed this again along with some other stuff
http://hg.mozilla.org/mozilla-central/rev/e6134e94c6cb

But there seemed to be an uptick in Tsvg_opacity numbers again. So maybe I didn't pin it on the right bug, or both bugs were responsible, or something else in this push is responsible. So I backed this out again to see.

http://hg.mozilla.org/mozilla-central/rev/ff9f542d0683
http://hg.mozilla.org/mozilla-central/rev/94dd26cec393

Comment 15

[Timothy Nikkel (:tnikkel)]

The numbers did not seem to go down after backout. Nothing else in the push should have affected Tsvg_opacity. I'll watch the numbers some more.

Comment 16

[Timothy Nikkel (:tnikkel)]

After watching the Talos numbers for Tsvg_opacity it looks like they go up and down without reason, sometimes differing by ~10% on the exact same changeset.

I landed this again and the numbers were in line with previous changesets, so it was all probably just part of the randomness of this test.
http://hg.mozilla.org/mozilla-central/rev/76240f6a38ae