Closed
Bug 105042
Opened 23 years ago
Closed 23 years ago
PR_sscanf() is 1% of startup
Categories
(Core :: XPConnect, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: dp, Assigned: jband_mozilla)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
PR_sscanf 1105 calls 31ms
The primary caller of this is:
xptiManifest::Read 99% 1077 calls 31ms
Can we do some easier parsing ?
Reporter | ||
Updated•23 years ago
|
Assignee | ||
Comment 1•23 years ago
|
||
You mean *less* easier parsing. Using PR_sscanf is easy. Writing a custom parser
is a pain.
Reporter | ||
Comment 2•23 years ago
|
||
Yeah. (or) Instead of being so ultra flexible, be more restrictive in what we
accept from the xpt files. After all, we are the ones writing it out too.
Assignee | ||
Comment 3•23 years ago
|
||
FWIW, this is not about xpt files. It is about xpti.dat (open it up and take a
look). It is highly structured (though the fix for bug 104191 adds a bit more
compexity). Writing a custom parser to handle the various data fields is
completely doable. It is just yet another chunk of code sucking up valuable
memory - and a pain :)
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•23 years ago
|
||
Reporter | ||
Comment 5•23 years ago
|
||
Ccing dougt for review of nsCRT::atoll()
The atoll() addition was not just for this. I have been wanting to use this in
the component manager etc for a very long time.
The dll size increased by 76 bytes.
Assignee | ||
Comment 6•23 years ago
|
||
dp! You should have said you were going to work on this. The bug was assigned
to me. I wrote something similar yesterday. It is entwined with another bug fix
- bug 104191. I'll attach the patch this afternoon after I get to MtView.
Reporter | ||
Comment 7•23 years ago
|
||
Sorry. I was up late last night and decide to do it (and reduce your pain!). No
sweat if you are not use my changes. They didnt take a lot of time anyway plus I
deserve it - I should have told you and only then worked on it.
Let me know if you need atoll() or you have another. I would need it for later
use too in plugins and xpcom-registry.
Reporter | ||
Comment 8•23 years ago
|
||
BTW, if would be good if you mark bug ASSIGNED and have a TFV in there.
Assignee | ||
Comment 9•23 years ago
|
||
Hey, I *did* accept the bug.
Anyway, I like your atoll better than what I wrote. I was trying to avoid the
64bit (and base 10) multiply by reading and writing the 64 bit numbers in hex
and scanning them in 32bit segments. What you have for that conversion is
probably fine. I'll integrate it into my patch.
I'll hack a little and post a patch. Thanks.
Assignee | ||
Comment 10•23 years ago
|
||
I posted my combined path to bug 104191.
I used dp's nsCRT::atoll. I used my own code to unify the scanning for
separators rather than the repeated calls to strchr. I also decided to store the
file size field as a uint32 and assume that the high 32bits are going to be zero
anyway.
I've been doing my profiling with Quantify and xpcshell.
xptiManifest::Read was taking over 20% of the *xpcom* startup time. With this
change it takes less than 10%.
Assignee | ||
Comment 11•23 years ago
|
||
The fix for this bug went into the trunk with the fix for bug 104191. Thanks dp!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 12•23 years ago
|
||
I do not have Quantify, etc. dp, would you be able to verify this fix for me?
Is the performance better now?
Thanks -
Reporter | ||
Comment 13•23 years ago
|
||
I will verify it.
Comment 14•23 years ago
|
||
dp reports that he has been able to verifiy this, so marking Verified -
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•