startup perf- remove the need of loading of maccharset.properties files at startup time to speed up

RESOLVED FIXED in mozilla0.9.6

Status

()

defect
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: ftang, Assigned: nhottanscp)

Tracking

({perf})

Trunk
mozilla0.9.6
PowerPC
Mac System 9.x
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

18 years ago
we currently load maccharset.properties at startup time, we should find a way to 
remove the need of loading it at startup time to speed up startup performance
we could 
1. build in common used pair in the cpp code, and/or
2. cache the resolved result in registry.

Updated

18 years ago
Blocks: 7251
Keywords: perf

Updated

18 years ago
Blocks: 71781
No longer blocks: 7251
per offline discussion with ftang - tentatively reassigning to myself
Assignee: ftang → jbetak
Reporter

Updated

18 years ago
Blocks: 103177
Reporter

Updated

18 years ago
Blocks: 95823
Reporter

Comment 2

18 years ago
mac sutff, move to ftang for now.
Assignee: jbetak → ftang
OS: All → Mac System 9.x
Hardware: All → Macintosh
Reporter

Comment 3

18 years ago
nhotta- can you handle this?

what we should do is
1. move the loading of property file from constructor to a seperate 
InitInfo() function
2. build in some common used mapping into c++ (for US, JA, DE and FR ) so we 
don't need to look at those mapping in the property file if we running on those 
tier 1 platform
3. call InitInfo right before we really need to access those mapping.
In this way, we keep both the dynamic part and tuning the performance at the 
same time. 
Assignee: ftang → nhotta
Target Milestone: --- → mozilla0.9.6
Assignee

Updated

18 years ago
Status: NEW → ASSIGNED
Reporter

Comment 6

18 years ago
-  PR_AtomicIncrement(&gCnt);
why you want to do that ? we should always have balance of

110   PR_AtomicDecrement(&gCnt); 
in the destructory. if you remove this you proably also need to change the 
destrcutor

+    PR_AtomicIncrement(&gCnt);
gCnt is the count of nsMacCharset object. it should be in the contstructor.

+    if (NS_FAILED(rv)) { charset.AssignWithConversion("x-mac-roman");}
please break the if statement into multiple line
don't put the body of the block and the if in the same line.
we won't be able to set break point in the body of the block
Reporter

Updated

18 years ago
Attachment #52611 - Attachment is obsolete: true
Reporter

Updated

18 years ago
Attachment #52618 - Attachment is obsolete: true
Attachment #52618 - Flags: review+
Attachment #52618 - Flags: needs-work+
Reporter

Comment 8

18 years ago
Comment on attachment 52649 [details] [diff] [review]
Patch, updated to ftang's comment.

look fine r=ftang
Attachment #52649 - Flags: review+
+    nsAutoString propertyURL(NS_LITERAL_STRING("resource:/res/
maccharset.properties"));
+    nsURLProperties *info = new nsURLProperties( propertyURL );

Not your fault, but nsURLProperties() needs to be updated to use some newer 
string stuff (e.g. is its argument read-only?).

+nsresult nsMacCharset::MapToCharset(short script, short region, nsString& 
charset)
Is 'charset' an out param? Rename it to make this clearer (e.g. 'outCharset').

+    rv = pMacLocale->GetPlatformLocale(&localeAsString, &script, &language, &
region);
Ugh, GetPlatformLocale takes a nsString*. Needs fixing later.

Rename 'charset' and sr=sfraser.
Assignee

Comment 11

18 years ago
Checked in to the trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

Comment 12

18 years ago
Changed QA contact to nhotta@netscap.com.
QA Contact: andreasb → nhotta
You need to log in before you can comment on or make changes to this bug.