74775 - GetTextExtentPoint32A called three times as often as necessary

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P1)

Description

[Daniel Bratell]

A couple of percent (~4%) of the time in the colour table stress case (see url) 
is caused by calls to GetTextExtentPoint32A which is an expensive function. I 
see the need to measure the text width but the problem is that the page only 
contains 10000 text frames (10000 texts inside table cells) and 
GetTextExtentPoint32A is called 29223 times. If it was to be called only once 
per text frame we could shave a couple of percent of the time.

The function is called:
20017 times from nsRenderingContextWin::GetWidth(char const*,UINT,int&)
          20000 times from nsTextFrame::MeasureText(...)
             17 times from nsTextFrame::PaintAsciiText(...)

 9204 times from nsRenderingContextWin::GetWidth(char const*,int,int,int 
*,int,int&,int&,int *)
           9204 times from nsTextFrame::MeasureText(...)

    2 times from nsFontMetricsWin::RealizeFont(void)


All these are cause by 29248 calls to nsTextFrame::Reflow(nsIPresContext 
*,nsHTMLReflowMetrics&,nsHTMLReflowState const&,UINT&).

I'm not sure where to start looking. Maybe we don't need to do 29248 reflows of 
the text frames? Maybe we can cache the width of the text so we don't have to 
remeasure it?

Comment 1

[Daniel Bratell]

Btw, it is 7% of the time for the simpler black&white test on 
http://www.mozilla.org/newlayout/testcases/stress/wbtbltxt.html

Comment 2

[Frank Tang]

erik resign. reassign all his bug to ftang for now.

Comment 3

[Frank Tang]

shanjian- can you take a look at this ?

Comment 4

[Shanjian Li]

GetWidth and MeasureText do not sounds problem. We probably should find out 
why reflow need to call MeasureText so many times. Reflow might also have 
better knowledge about what data can be cache. 

reassign it to Chris Karnaze for further reassignment. 

If you believe the improvement should be made inside MeasureText, you can 
reassign back to me.

Comment 5

[anthonyd]

not a table specific bug, reassigning back to owner.

Comment 6

[Marc Attinasi]

Looks liek a table issue to me. Load this in Viewer with 'Reflow Counts On' and
look at the cells - the ones at the top are getting 2 or 3 reflows, whereas the
cells at the end are getting 1. Why are the top row's cells getting multiple
reflows?

Comment 7

[karnaze (gone)]

The top cells are reflowed 3 times, once for an initial unconstrained reflow, 
once when the table is first balanced with known column widths, and once due to 
a rebalancing caused by the middle cells comming in (due to incremental page 
loading) after the initial balancing and causing the table to change. The middle 
cells are getting reflowed 2 times - an initial reflow when the column widths 
were established after the 1st balance and an additional reflow when the column 
widths change because they were added. The bottom cells are reflowed 1 time 
because they come in after the table columns are established and don't change 
the column widths. I'm happy to report that this illustrated that table reflow 
is being properly optimized. 

I'm not sure what to do with this bug.

Comment 8

[karnaze (gone)]

-->attinasi for a possible optimization in block code.

Comment 9

[Daniel Bratell]

Maybe cache the result of the text measurements between reflows and throw it
away when the table has ended?

Comment 10

[karnaze (gone)]

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

Comment 11

[karnaze (gone)]

Kevin and I measured the number of times GetTextExtendPoint32A is getting called 
in the testcase, a shortened version of the test case and a test case with 
<div>s. It appears to be getting called 3 times the number of reflows (e.g. a 
table with 100 cells each getting reflowed 2 times would result in 600 calls).

Comment 12

[Daniel Bratell]

Have you looked at the strings measured? I think I've seen (reported?) empty
strings or whitespace strings being measured. In the original tables there are
no whitespace. 

i.e. it could be the result of measuring whitespace texts between tags. On the
other hand, it could a really promising bug. Text measurements are expensive.

Comment 13

[Marc Attinasi]

Moving out to 1.0.1 - this is a _potential_ performance win, but there is no
evidence to suggest that it will be significant.

Comment 14

[Marc Attinasi]

Moving Mozilla 1.01 bugs to 'future' milestone with priority P1

I will be pulling bugs from 'future' milestones when scheduling later work.

Comment 15

[leon.zhang]

Daniel, I have tested your testcase in win2k with rc1.
It seems to me that GetTextExtentPoint32A is never called during pageloading.
I think this bug can be closed.

can you verify it?

Comment 16

[Daniel Bratell]

Never ever? Then how is text size measured?

Comment 17

[Jerry]

daniel, I set two breakpoint at the lines where it calls GetTextExtentPoint32A
functions, then load the two testcase,
vc debugger doesnot stop at the breakpoints.

and this bug is reported one years ago.
So I suspect maybe this bug is not valid now.

Comment 18

[Daniel Bratell]

I'm not sure. The reason that we called GetTextExtentPoint32A three times was
that we measured text width three times. If we now measure text width some other
way, we probably still measure it three times. Who knows about text measurements?

Have you tried setting a breakpoint at all these places:
http://lxr.mozilla.org/seamonkey/search?string=GetTextExtent

Comment 19

[Jerry]

No, I dont set breakpoint as you said.
I just set breakpoint according to the search result of
http://lxr.mozilla.org/seamonkey/search?string=GetTextExtentPoint32A

Comment 20

[Daniel Bratell]

Could you try the other GetTextExtentPoint functions too? There's some magic to
decide which to use that depends on platform and such so maybe it's
GetTextExtentPoint32W that's used when you're testing. Because someting _has_ to
be  used when we're measuring how large text is.

Comment 21

[Jerry]

maybe we should focus on how to reduce the use of MeasureText() in reflow,
instead of focus on use of GetTextExtentPoint32A(), for MeasureText() is for all
platform.

Daniel, you said can we cache the result of MeasureText() during reflow process
for one textframe, yes, I agree.

So next we should make clear 
during reflow, is it possible to cache the result of MeasureText() in initial
reflow   for other reflow?

Comment 22

[leon.zhang]

I think the same phenomenon existed in the solaris platform,
i.e. there are tree kind of reflow count matched with the three parts of the 
big table.

I found that, in environment(solris8 + mozilla1.0RC2),the calling function
is: nsRenderingContextGTK::GetTextDimensions-->GetWidth

so I think we should focus nsTextFrame::MeasureText
and not GetWidth which is related to the OS platform.

Comment 23

[leon.zhang]

I have changed the function nsTextFrame::reflow(),and cached the result of 
function nsTextFrame::MeasureText,then the calling number of function 
MeasureText reduced to only once for each TextFrame when applied the modified
browser to the orignal long table testcase..

But cause a Assertion in nsBlockFrame::ReflowInlineFrame when applied to the 
ordinaryl webpage.

I do not know the relationship between nsTextFrame and nsBlockFrame.

Comment 24

[Daniel Bratell]

What assertion do you get?

Comment 25

[leon.zhang]

in file "nsBlockFrame.cpp":

NS_ASSERTION(breakType != NS_STYLE_CLEAR_NONE, "bad break type");
NS_ASSERTION(NS_STYLE_CLEAR_PAGE != breakType, "no page breaks yet");

when the above ASSERTION happen?

Comment 26

[Daniel Bratell]

I guess your cache changed the size of nsTextFrame? Have you made sure that you
recompiled _everything_ that uses or depends on the size?

Comment 27

[leon.zhang]

I have known the reason why the ASSERTION appeared.
Because the cached content is not complete.
I'll give a patch about the bug later.

Comment 28

[leon.zhang]

The modified source code can work well when applied to normal webpage(include 
mozilla website)

but when visit "news.sina.com.cn",meet info below:


warning: property cp1256 already exists

###!!! ASSERTION: no body node: 'bodyContent', file G:\mozilla-trunk\mozi
lla1.0rc1\mozilla\layout\html\style\src\nsCSSRendering.cpp, line 2469
###!!! Break: at file G:\mozilla-trunk\mozilla1.0rc1\mozilla\layout\html\style\s
rc\nsCSSRendering.cpp, line 2469

when will info above appear?

Comment 29

[Daniel Bratell]

That might be an old assertion. It's mentioned in bug 136959 comment 1. Check
with a clean build to see if it's there too and if it is, it's not a problem
with your new code.

Comment 30

[leon.zhang]

Attachment: cahced the result of MeasureText

hi,all

I modified the code of	the function nsTextFrame::Reflow in nsTextFrame.cpp
cached the result of calling nsTextFrame::MeasureText.

I also tested it in windows2000 and solaris 8,works well.

If you tested the patch, please give me your suggestion.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 88270 [details] [diff] [review]
cahced the result of MeasureText

This patch adds an extra 12 bytes per text frame, plus an extra heap allocation
of an nsTextTransformer which is both a performance and memory hit, presumably.
 I would be quite surprised if this makes things faster rather than slower. 
I'd also expect it would introduce a bunch of bugs, since it doesn't look like
there's anything to invalidate the cache when we do something like resize a
page.

This is the type of thing that would require significant discussion among the
layout module owners and peers before we would want to do it.  However, I
suspect the answer would be that we wouldn't.  If the result is really going to
be the same each time, perhaps something at a much higher level should be
caching the result, such as something in the table code.  However, I'd rather
wait to work on this until nsIFrame layout is using GetMinSize / GetPrefSize
like nsIBox layout is.

Comment 32

[leon.zhang]

hi DBaron 

I want to know that the thought of caching the result of calling MeasureText 
is useful or feasible. I think caching process is to prevent from calling the 
lower functions, which are related to the diffirent OS, many times. 

As for the content of the cache, we can optimize it further.At same time,the 
using model of the cache can been enhanced. But the most important is whether 
the thought of caching is feasible.

Currently,my thought is that if the content of the nxTextFrame is not changed,
then the cache is valuble. So, refreshing the cache's content is not necessary 
if only the frame being resized.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It doesn't seem useful to me.  Others may think otherwise, though.

Comment 34

[leon.zhang]

Is there similar or related BUG which can help us to resolve this one?

Comment 35

[leon.zhang]

TestCase1 and TestCase2 are same to the testcase in mozilla's(
http://www.mozilla.org/newlayout/testcases/stress/wbtbltxt.html
http://www.mozilla.org/newlayout/testcases/stress/wbtblclr.html).


TestCase1:  
http://boxing.prc/testcases/others/wbtbltxt_n.html

	Modified       Original
	----------------------	  
	2937		3097
	3031		2829
	3110		2849
	3015		3015
	3125		3000
	3391		2969
	3141		3016
	3031		3265
	3156		2953
	3141		2953
	3125		2937
	3078		2937
	3141		2938
	----------------------
Average	3109.38		2981.38


TestCase2:  
http://boxing.prc/testcases/others/wbtblclr_n.html

	Modified       Original  
	----------------------
	5172		5016
	5250		5313
	5250		5032
	5297		5031
	5375		4843
	5406		5125
	5531		4859
	5312		4734
	5250		4735
	5515		4843
	4984		4703
	5125		4813
	4869		4703
	----------------------		
Average	5256.62		4903.85

note:
    "Modified " means the version of mozilla have applied the thought
of caching the result of NsTextFrame::MeasureText.
    "Original" means the orignal mozilla which was not modified.


According to the testing data above, we can find that the the data in part 
of "Modified" 
is bigger than the "Original" part,i.e.,caching the result of function 
MeasureText is 
not effective to enhance the performance of mozilla when mozilla process long 
and big table.

In addition, the caching process import extra heap memory allocation and other
pointers which also cost 12 bytes for each TextFrame(see comments #31).

In a word, caching the result of MeasureText cost more resource(CPU time and 
memeory) 
than keeping the original processing method.

Up to now, I don't think this BUG is a big of mozilla, 
so I suggest that the bug be closed because of analyzing above.

Comment 36

[leon.zhang]

ref comments #35

Comment 37

[leon.zhang]

notes:
     tesing data in comments #35 is the consuming time when access the two
testcase (html file).

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Just because one optimization doesn't work doesn't mean there aren't ways that
we could improve the number of times we do text measurement.  I'm not sure if
it's the best idea to have a general bug on it, but I think it's something worth
considering.