Closed
Bug 96530
Opened 23 years ago
Closed 23 years ago
delay loading of unicharutil in nsTextTransformer::Initialize
Categories
(Core :: Layout, defect, P3)
Core
Layout
Tracking
()
VERIFIED
FIXED
mozilla0.9.6
People
(Reporter: ftang, Assigned: ftang)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
patch
|
dp
:
review+
waterson
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Comment 2•23 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•23 years ago
|
||
dp and waterson- can you r/sr= this one (for m0.9.5)
Comment 4•23 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•23 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 | ||
Comment 6•23 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•23 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•23 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•23 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•23 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 | ||
Comment 11•23 years ago
|
||
waterson, I think the latest patch is fine. do you have more concern ?
Assignee | ||
Updated•23 years ago
|
Comment 12•23 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•23 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•23 years ago
|
||
True. But you _still_ haven't answered my question. Does this make a difference
for our startup performance? :-)
Comment 15•23 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•23 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•23 years ago
|
||
Okay, I'm sold. :-)
Comment 18•23 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•23 years ago
|
Assignee | ||
Comment 19•23 years ago
|
||
fixed and check in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•