Closed
Bug 129167
Opened 19 years ago
Closed 19 years ago
nsTextFrame calls memmove _way_ too much for long lines in <pre>
Categories
(Core :: Layout, defect, P3)
Tracking
()
VERIFIED
FIXED
People
(Reporter: bzbarsky, Assigned: smontagu)
References
()
Details
(Keywords: perf)
Attachments
(3 files, 2 obsolete files)
251.47 KB,
text/html
|
Details | |
227.16 KB,
text/html
|
Details | |
2.55 KB,
patch
|
smontagu
:
review+
attinasi
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
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()
Assignee | ||
Updated•19 years ago
|
Comment 2•19 years ago
|
||
nsbeta1+ for now. simon- do you have any eta?
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•19 years ago
|
||
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.
![]() |
Reporter | |
Comment 4•19 years ago
|
||
![]() |
Reporter | |
Comment 5•19 years ago
|
||
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).
Assignee | ||
Comment 6•19 years ago
|
||
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.
Assignee | ||
Comment 7•19 years ago
|
||
The same idea, but with nsDeque (thanks, dbaron)
Attachment #74269 -
Attachment is obsolete: true
![]() |
Reporter | |
Comment 8•19 years ago
|
||
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.
Assignee | ||
Comment 9•19 years ago
|
||
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.
![]() |
Reporter | |
Comment 10•19 years ago
|
||
Simon, where are you seeing it take ages to close viewsource? I'm not seeing that at all here (with your nsDeque patch applied).
Assignee | ||
Comment 11•19 years ago
|
||
I no longer see a problem closing view source window in an updated build, so it was probably just a glitch
Comment 12•19 years ago
|
||
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+
Assignee | ||
Comment 13•19 years ago
|
||
Attachment #74457 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #74808 -
Flags: review+
Updated•19 years ago
|
Attachment #74808 -
Flags: superreview+
Comment 14•19 years ago
|
||
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...
Assignee | ||
Comment 15•19 years ago
|
||
I applied the patch to another tree and don't see the 'glitch' there either, so I am sure :-)
Comment 16•19 years ago
|
||
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+
Assignee | ||
Comment 17•19 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 18•19 years ago
|
||
do we SEE the performance improvement after we land ?
Comment 19•19 years ago
|
||
can any one verify perf improvement..?
![]() |
Reporter | |
Comment 20•19 years ago
|
||
I see it on big enough view-source testcases when that 16% I mention becomes big enough to measure by wall clock. :)
![]() |
Reporter | |
Comment 22•18 years ago
|
||
*** 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.
Description
•