769504 - Atomize strings when adding to Map (as a key) or Set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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]]

Attachment: part 0 - trivial: switch to // comments

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - atomize, v1

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 638358 [details] [diff] [review]
part 1 - atomize, v1

Nice

Comment 4

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/926876345186
https://hg.mozilla.org/integration/mozilla-inbound/rev/aec1ad4171a1

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/926876345186
https://hg.mozilla.org/mozilla-central/rev/aec1ad4171a1

Comment 6

[(no longer active)]

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]]

Relanded. https://hg.mozilla.org/integration/mozilla-inbound/rev/e5345853fdac

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e5345853fdac