Last Comment Bug 769504 - Atomize strings when adding to Map (as a key) or Set
: Atomize strings when adding to Map (as a key) or Set
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on: 743107
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 16:48 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-08-02 19:08 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 0 - trivial: switch to // comments (5.41 KB, patch)
2012-07-02 07:55 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review
part 1 - atomize, v1 (2.36 KB, patch)
2012-07-02 07:56 PDT, Jason Orendorff [:jorendorff]
luke: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-06-28 16:48:33 PDT
HashableValue::setValue is a convenient place to do this.

Tradeoffs:

- Atomizing takes time.
+ Without atomizing, every hash table lookup with a string key
  must take time proportional to the length of the string.
  With atomizing, there are some cases where lookup can be much faster.
+ With atomizing, all comparisons are simple 64-bit comparisons.
+ With atomizing, HashableValue::hash() is always fast, so growing a table
  can be fast even if there are long string keys.
+ With atomizing, HashableValue::hash() is so fast that caching a hash code
  for each entry in the hash table is pointless. (We currently do not cache
  hash codes anyway; we're just slow.)
- Atomizing can make extra copies of strings.
+ Atomizing can common up strings, saving memory.
- Atomizing makes strings in the atom compartment that won't be collected
  until the next full (runtime-wide) GC.

I think we should atomize.

(At the same time--I think HashableValue::hash()'s last line can be a little simpler given the changes to ScrambleHashCode in bug 743107. Not sure.)
Comment 1 Jason Orendorff [:jorendorff] 2012-07-02 07:55:33 PDT
Created attachment 638357 [details] [diff] [review]
part 0 - trivial: switch to // comments
Comment 2 Jason Orendorff [:jorendorff] 2012-07-02 07:56:02 PDT
Created attachment 638358 [details] [diff] [review]
part 1 - atomize, v1
Comment 3 Luke Wagner [:luke] 2012-07-02 09:19:50 PDT
Comment on attachment 638358 [details] [diff] [review]
part 1 - atomize, v1

Nice
Comment 6 :Ehsan Akhgari 2012-07-04 16:56:09 PDT
I was backing out a number of patches in the js land, and unfortunately this patch caused a number of merge conflicts which I did not trust myself to resolve.  Therefore, I had to back it out.  Please reland after fixing the conflicts resulting from the backouts.  Thanks, and apologies for the inconvenience.

Backout changeset:
https://hg.mozilla.org/mozilla-central/rev/f6bdb7fc663f
Comment 7 Jason Orendorff [:jorendorff] 2012-08-02 13:16:53 PDT
Relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/e5345853fdac
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-08-02 19:08:06 PDT
https://hg.mozilla.org/mozilla-central/rev/e5345853fdac

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