delay loading of unicharutil in nsTextTransformer::Initialize

VERIFIED FIXED in mozilla0.9.6

Status

()

Core
Layout
P3
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Frank Tang, Assigned: Frank Tang)

Tracking

({perf})

Trunk
mozilla0.9.6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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

Updated

16 years ago
Blocks: 75041
(Assignee)

Comment 1

16 years ago
Created attachment 46903 [details] [diff] [review]
delay loading of case conversion so we can speed up startup time.
(Assignee)

Comment 2

16 years ago
with the patch in 96529, we can remove the loading of unicharutil in the -chome
blank.xul
Status: NEW → ASSIGNED
(Assignee)

Comment 3

16 years ago
dp and waterson- can you r/sr= this one (for m0.9.5) 
Keywords: perf
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5

Comment 4

16 years ago
Yeah but when is it actually loaded before and after ? Is there a big diff from
when the nsTextTransformer gets created and used ?
(Assignee)

Comment 5

16 years ago
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.
(Assignee)

Updated

16 years ago
Blocks: 103175
(Assignee)

Comment 6

16 years ago
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 ?
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Comment 7

16 years ago
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.
Attachment #46903 - Flags: review+
(Assignee)

Comment 8

16 years ago
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

16 years ago
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.
(Assignee)

Comment 10

16 years ago
>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. 
(Assignee)

Updated

16 years ago
Blocks: 104056
(Assignee)

Comment 11

16 years ago
waterson, I think the latest patch is fine. do you have more concern ?
(Assignee)

Updated

16 years ago
Blocks: 104148
No longer blocks: 104056

Comment 12

16 years ago
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. :-)
(Assignee)

Comment 13

16 years ago
>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

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

Comment 15

16 years ago
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

16 years ago
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

16 years ago
Okay, I'm sold. :-)

Comment 18

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

sr=waterson
Attachment #46903 - Flags: superreview+
(Assignee)

Updated

16 years ago
Blocks: 104060
No longer blocks: 104148
(Assignee)

Comment 19

16 years ago
fixed and check in 
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Updated

16 years ago
No longer blocks: 104060

Comment 20

16 years ago
Marking verified in the Oct 22nd build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.