Closed Bug 96530 Opened 23 years ago Closed 23 years ago

delay loading of unicharutil in nsTextTransformer::Initialize

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.6

People

(Reporter: ftang, Assigned: ftang)

References

Details

(Keywords: perf)

Attachments

(1 file)

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
Blocks: 75041
with the patch in 96529, we can remove the loading of unicharutil in the -chome blank.xul
Status: NEW → ASSIGNED
dp and waterson- can you r/sr= this one (for m0.9.5)
Keywords: perf
Priority: -- → P3
Target Milestone: --- → mozilla0.9.5
Yeah but when is it actually loaded before and after ? Is there a big diff from when the nsTextTransformer gets created and used ?
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.
Blocks: 103175
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 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+
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.
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.
>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.
Blocks: 104056
waterson, I think the latest patch is fine. do you have more concern ?
Blocks: 104148
No longer blocks: 104056
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. :-)
>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.
True. But you _still_ haven't answered my question. Does this make a difference for our startup performance? :-)
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)
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.
Okay, I'm sold. :-)
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+
Blocks: 104060
No longer blocks: 104148
fixed and check in
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
No longer blocks: 104060
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.

Attachment

General

Creator:
Created:
Updated:
Size: