Last Comment Bug 676700 - Slightly optimize js::IndexToId
: Slightly optimize js::IndexToId
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- minor (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2011-08-04 15:18 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-05 09:07 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (1.88 KB, patch)
2011-08-04 15:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
v2 (5.63 KB, patch)
2011-08-04 17:56 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 15:18:34 PDT
Created attachment 550851 [details] [diff] [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.
Comment 1 User image Luke Wagner [:luke] 2011-08-04 16:42:39 PDT
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 User image Luke Wagner [:luke] 2011-08-04 16:43:45 PDT
If nothing else, it seems like we should have an inline reusable function to factor out the core backwards-filling index-to-chars algorithm.
Comment 3 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 17:56:27 PDT
Created attachment 550912 [details] [diff] [review]

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.
Comment 4 User image Luke Wagner [:luke] 2011-08-04 18:09:29 PDT
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 User image Luke Wagner [:luke] 2011-08-04 18:11:07 PDT
However, if the input is already atomized, then the new string is still garbage, so your approach is better.
Comment 6 User image Luke Wagner [:luke] 2011-08-04 18:26:42 PDT
Comment on attachment 550912 [details] [diff] [review]

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?
Comment 7 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-04 23:29:02 PDT
Comment 8 User image Marco Bonardo [::mak] 2011-08-05 09:07:58 PDT

Note You need to log in before you can comment on or make changes to this bug.