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

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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()

Comment 1

[Christine Hoffman]

Changing QA contact

Comment 2

[Frank Tang]

nsbeta1+ for now. 
simon- do you have any eta?

Comment 3

[Simon Montagu :smontagu]

Attachment: A possible approach

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.

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: baseline

Comment 5

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Attachment: 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).

Comment 6

[Simon Montagu :smontagu]

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.

Comment 7

[Simon Montagu :smontagu]

Attachment: New patch using nsDeque

The same idea, but with nsDeque (thanks, dbaron)

Comment 8

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 9

[Simon Montagu :smontagu]

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.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Simon, where are you seeing it take ages to close viewsource?  I'm not seeing
that at all here (with your nsDeque patch applied).

Comment 11

[Simon Montagu :smontagu]

I no longer see a problem closing view source window in an updated build, so it
was probably just a glitch

Comment 12

[timeless]

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

Comment 13

[Simon Montagu :smontagu]

Attachment: With¦the¦change¦timeless¦requested

Comment 14

[Marc Attinasi]

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...

Comment 15

[Simon Montagu :smontagu]

I applied the patch to another tree and don't see the 'glitch' there either, so
I am sure :-)

Comment 16

[Asa Dotzler [:asa]]

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

Comment 17

[Simon Montagu :smontagu]

Fix checked in.

Comment 18

[Frank Tang]

do we SEE the performance improvement after we land ?

Comment 19

[Moied]

can any one verify perf improvement..?

Comment 20

[Boris Zbarsky [:bzbarsky, bz on IRC]]

I see it on big enough view-source testcases when that 16% I mention becomes 
big enough to measure by wall clock.  :)

Comment 21

[Moied]

verified

Comment 22

[Boris Zbarsky [:bzbarsky, bz on IRC]]

*** Bug 74833 has been marked as a duplicate of this bug. ***