Closed
Bug 676700
Opened 13 years ago
Closed 13 years ago
Slightly optimize js::IndexToId
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: Waldo, Assigned: Waldo)
Details
(Whiteboard: [inbound])
Attachments
(1 file, 1 obsolete file)
5.63 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Sparse/dense patchwork needed js::IndexToId, which I then implemented, before discovering it had already been done. But the current implementation isn't as efficient as this one (it creates an intermediate string rather than atomize directly from a jschar buffer), so let's improve it slightly.
Attachment #550851 -
Flags: review?(luke)
Comment 1•13 years ago
|
||
IIRC, js_AtomizeString(js_IntToString()) is only a few branches (and obviously a function call) worse than the proposed IndexToId. Can you measure the perf improvement for any use of IndexToId? Btw, there is a flagrantly redundant branch in js_AtomizeString perhaps you could sweep up...
Comment 2•13 years ago
|
||
If nothing else, it seems like we should have an inline reusable function to factor out the core backwards-filling index-to-chars algorithm.
Assignee | ||
Comment 3•13 years ago
|
||
It's not the js_AtomizeString call that contributes extra time. It's that js_NumberToString must allocate a new JSString, and then that JSString immediately becomes garbage after the atom is created. So a little time, a little more GC pressure, neither particularly great (if not too horrible generally -- although, one of the JSON parser optimizations I did was to eliminate an extra string creation like this, and it showed up in perf numbers). I was thinking that loop was a bit duplicated, but it looked a little annoying to abstract it out. This patch does that.
Attachment #550851 -
Attachment is obsolete: true
Attachment #550851 -
Flags: review?(luke)
Attachment #550912 -
Flags: review?(luke)
Comment 4•13 years ago
|
||
Ohhh, I knew Atomize converted non-atom strings to JSAtoms, but I forgot that it doesn't do this to string argument passed to js_AtomizeString. It seems like it should. A quick scan of js_AtomizeString shows at least a couple of places that exhibit the immediately-garbage string pattern you just mentioned.
Comment 5•13 years ago
|
||
However, if the input is already atomized, then the new string is still garbage, so your approach is better.
Comment 6•13 years ago
|
||
Comment on attachment 550912 [details] [diff] [review] v2 Review of attachment 550912 [details] [diff] [review]: ----------------------------------------------------------------- Wow, I didn't realize it was x3! r+ with nits. ::: js/src/jsatominlines.h @@ +142,5 @@ > + * than end. > + */ > +template <typename T> > +inline mozilla::RangedPtr<T> > +WriteIndex(uint32 index, mozilla::RangedPtr<T> end) As much as I like short names, this seems too short for how weird it is. How about BackfillIndexInCharBuffer? Second idea: how about a symbolic UINT32_CHAR_BUFFER_LENGTH constant that is statically asserted correct here and can be reused/asserted in the 3 other places?
Attachment #550912 -
Flags: review?(luke) → review+
Assignee | ||
Comment 7•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/699b68990d54
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/699b68990d54
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•