96530 - delay loading of unicharutil in nsTextTransformer::Initialize

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Frank Tang]

this is a spin off of 75041, I find out one of the reason unicharutil got load
into memory is because nsTextTransformer::Initialize get case conversion.
I will include a patch to address this later and let attarnasi review

Comment 1

[Frank Tang]

Attachment: delay loading of case conversion so we can speed up startup time.

Comment 2

[Frank Tang]

with the patch in 96529, we can remove the loading of unicharutil in the -chome
blank.xul

Comment 3

[Frank Tang]

dp and waterson- can you r/sr= this one (for m0.9.5) 

Comment 4

[Suresh Duddi (gone)]

Yeah but when is it actually loaded before and after ? Is there a big diff from
when the nsTextTransformer gets created and used ?

Comment 5

[Frank Tang]

This object won't be needed untill we hit any 

text-transform: uppercase|lowercase|capitcalize  

in CSS .I don't think we have any right now. 

add jbetak to the cc list.

Comment 6

[Frank Tang]

nsTextTransformer is needed when you try to layout text in any ml inside layout. 
It is create in the nsTextFrame::Reflow routine. 

Before the change, the nsTextTransformer::Initialize is called in the module 
loading time. After the change, the EnsureCaseConv (loading of the unicharutil ) 
is called when we display some text (inside nsTextFrame::Reflow() ) which have
text-transform: uppercase|lowercase|capitcalize
in css. 
dp- can you r= this one ?

Comment 7

[Suresh Duddi (gone)]

Comment on attachment 46903 [details] [diff] [review]
delay loading of case conversion so we can speed up startup time.

Hope there no other place where gCaseConv is used.

Curious what happens if EnsuerCaseConv() fails for come reason. Make sure this case is well handled in each of the areas.
r=dp otherwise.

Comment 8

[Frank Tang]

if EnsuerCaseConv failed, we will get assertion from the existing code
108     NS_ASSERTION( NS_SUCCEEDED(res), "cannot get UnicharUtil");
109     NS_ASSERTION( gCaseConv != NULL, "cannot get UnicharUtil");

and we won't execute the in buffer replacement toupper or to lower code. 

Comment 9

[Chris Waterson]

Does this really make that much of a difference? (Does it make any difference?)
We're polluting the unicode text layout path here: we ought to be able to
convince ourselves that this is worth it.

Comment 10

[Frank Tang]

>Does this really make that much of a difference? (Does it make any difference?)
Not sure . I only know with this one and the xpcom case conversion one, could 
remove the need of loaind this dll at startup time. 

>We're polluting the unicode text layout path here: we ought to be able to
>convince ourselves that this is worth it.
not quite sure what does this mean. We won't polluting the unciode text layout 
path. We will only delay the loading till we really need it. and the check is 
not normailly need. we only need that while we have attribute like 
text-transform, which is not usually employeed for normal text. 

Comment 11

[Frank Tang]

waterson, I think the latest patch is fine. do you have more concern ?

Comment 12

[Chris Waterson]

What I meant was, we are adding extra function call overhead in several places
when we're going to be reflowing unicode text. (And due to bug 77585, often
ASCII text is bogously inflated to Unicode.) I'd rather slow down start up if it
means faster page loading. So...

1. Tell me how much this improves startup performance. Maybe dp can help you
   with that.

2. Tell me how much this slows down page load performance. Run jrgm's page
   load tests (<http://cowtools.mcom.com>) on an optimized build and collect
   before and after numbers.

I don't mean to be a stick in the mud here; I'm just not convinced that this is
worth doing. :-)

Comment 13

[Frank Tang]

>What I meant was, we are adding extra function call overhead in several places
>when we're going to be reflowing unicode text.
Yes, I understand, but those extra function call overhead WON'T got called
unless there are css 
text-transform: lowercase;
text-transform: uppercase;
text-transform: capital;

in other words, usually these extra function call won't got called because most
of the page does NOT include such css.

Comment 14

[Chris Waterson]

True. But you _still_ haven't answered my question. Does this make a difference
for our startup performance? :-)

Comment 15

[Suresh Duddi (gone)]

Chris, if you are concerned about the extra fn. call, then we could change the
patch to instead of doing

+          if(NS_SUCCEEDED(EnsureCaseConv()))
+            gCaseConv->ToLower(result, result, wordLen);

to

+          if (!gCaseConv) EnsureCaseConv();
+          gCaseConv->ToLower(result, result, wordLen);

For startup it was a goal to eliminate unneccessary dll loads as one of the
earlier analysis said dll loads are 1/2 of startup. (Will add perf improvement
of this patch one my build finishes)

Comment 16

[Suresh Duddi (gone)]

Total startup time: 4.246 sec
00004.246: ...main1

Time to load unicharutils (ucharuti.dll) : (0.641 - 0.620 ) = 0.021 sec
00001.151:    PR_LoadLibrary total: 0.620
...
Loading: e:\dp\trunk\mozilla\dist\WIN32_O.OBJ\bin\components\ucharuti.dll
00001.182:     PR_LoadLibrary total: 0.641

Lower bound on time consumed by loading ucharuti.dll : 0.021/4.246 * 100 = 0.5%

Remember this is a lower bound (this could be higher) as the timing is purely
the PR_LoadLibrary time, the total startup time is computed as all of main1
(startup will be a little lesser)...

I would love to take this Chris for startup.

Comment 17

[Chris Waterson]

Okay, I'm sold. :-)

Comment 18

[Chris Waterson]

Comment on attachment 46903 [details] [diff] [review]
delay loading of case conversion so we can speed up startup time.

sr=waterson

Comment 19

[Frank Tang]

fixed and check in 

Comment 20

[Chris Petersen]

Marking verified in the Oct 22nd build.