Closed Bug 129167 Opened 18 years ago Closed 18 years ago

nsTextFrame calls memmove _way_ too much for long lines in <pre>

Categories

(Core :: Layout, defect, P3)

x86
Linux
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: bzbarsky, Assigned: smontagu)

References

()

Details

(Keywords: perf)

Attachments

(3 files, 2 obsolete files)

Spun off bug 98118.  Now that we no longer call into the linebreaker, the
biggest perf culprit is the memmove() call we get from ForgetWordFrames()
twiddling arrays.  12% of time for loading the source of http://www.toronto.com
is spent inside memmove()
Blocks: 84707
Keywords: nsbeta1, perf
Changing QA contact
QA Contact: petersen → moied
Priority: -- → P3
nsbeta1+ for now. 
simon- do you have any eta?
Keywords: nsbeta1nsbeta1+
Status: NEW → ASSIGNED
Attached patch A possible approach (obsolete) — Splinter Review
In my testing this patch makes |ForgetWordFrame()| itself 15% faster, but
doesn't seem to impact reflow as a whole. I would be interested in what jprof
shows.
Attached file baseline
Attached file With the patch applied
Short story:  

Jprof now shows no hits at all inside ForgetWordFrame on this testcase (used to
be 16% of the pageload).

Time to load source on that url went down by about 16%.  (which agrees)

Time spent in MeasureText went down 48% (yes, about a factor of two).
I'm taking that as a 'yes' for the concept, at least, but a better implentation
would be to use a nsDeque instead of the nsAutoVoidArray.
Attached patch New patch using nsDeque (obsolete) — Splinter Review
The same idea, but with nsDeque (thanks, dbaron)
Attachment #74269 - Attachment is obsolete: true
Yep.  That has pretty much the same effect on the profile.  I should really try
shaver's 8khz patch; these profiles are starting to get statistically a little
bogus.
What I don't like with the nsDeque version is that (on Linux) it takes ages to 
close the view-source window. I am wondering if there is a problem with the 
nsDeque destructor.
Simon, where are you seeing it take ages to close viewsource?  I'm not seeing
that at all here (with your nsDeque patch applied).
I no longer see a problem closing view source window in an updated build, so it
was probably just a glitch
Comment on attachment 74457 [details] [diff] [review]
New patch using nsDeque

swap these two lines:
+  NS_ASSERTION((void*)aFrame == mWordFrames.PeekFront(), "forget-word-frame");
+  if (0 != mWordFrames.GetSize()) {
[and fix indentation] for r=timeless
Attachment #74457 - Flags: review+
Attachment #74457 - Attachment is obsolete: true
Attachment #74808 - Flags: review+
Attachment #74808 - Flags: superreview+
Comment on attachment 74808 [details] [diff] [review]
With¦the¦change¦timeless¦requested

sr=attinasi, assuming that you are sure the 'glitch' of the view source window
closing is gone...
I applied the patch to another tree and don't see the 'glitch' there either, so
I am sure :-)
Comment on attachment 74808 [details] [diff] [review]
With¦the¦change¦timeless¦requested

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74808 - Flags: approval+
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
do we SEE the performance improvement after we land ?
can any one verify perf improvement..?
I see it on big enough view-source testcases when that 16% I mention becomes 
big enough to measure by wall clock.  :)
verified
Status: RESOLVED → VERIFIED
*** Bug 74833 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.