Closed Bug 222436 Opened 19 years ago Closed 13 years ago

GetBidiEnable call is very expensive

Categories

(Core :: Layout, defect)

x86
Windows NT
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ire0, Assigned: tnikkel)

Details

(Keywords: perf)

Attachments

(2 files)

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.
Note, the second test was a large text document.  

The BidiEnable switch
should be cached to prevent so many calls to get it.  
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.
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.
Keywords: perf
bryner, are there any cases when an nsPresContext will have an mShell but that
won't have a document?
Assignee: layout → nobody
QA Contact: ian → layout
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.
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.
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.
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.
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.
Attached patch patchSplinter Review
Ok, let's do that.
Assignee: nobody → tnikkel
Attachment #437722 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/3a27158cbdd6
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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.
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.