Shark was showing .9% of total SS time in JS_DHashTableOperate under js_AtomizePrimitive. The attached patch replaces JSDHashTable with js::HashSet and drops about 4ms on SS. Also, with the help of Ranges and relookupOrAdd, it simplifies the code quite a bit, removing almost 100 lines.

Yay! ++lw /be

Comment on attachment 460190 [details] [diff] [review] patch >diff --git a/js/src/jsatom.cpp b/js/src/jsatom.cpp >+static JS_ALWAYS_INLINE AtomEntryType >+StringToInitialAtomEntry(JSString *str) >+{ >+ return (AtomEntryType)str; >+} Nit: s/static JS_ALWAYS_INLINE/inline here and everywhere in the patch. Add a blank between the cast () and str. >+AtomEntryFlags(AtomEntryType entry) >+{ >+ return (uintN)(entry & ATOM_ENTRY_FLAG_MASK); >+} Nit: add a space between the cast () and (entry. >+static JS_ALWAYS_INLINE void >+AddAtomEntryFlags(const AtomEntryType &entry, uintN flags) > { >+ const_cast<AtomEntryType &>(entry) |= flags; > } >+static JS_ALWAYS_INLINE void >+ClearAtomEntryFlags(const AtomEntryType &entry, uintN flags) > { >+ const_cast<AtomEntryType &>(entry) &= ~(uintN)flags; Nit: flags is uintN so no need to cast. > > JSBool > js_InitAtomState(JSRuntime *rt) > { >+ * The caller must zero the state before calling this function. We depend on >+ * the implementation detail that a zero'd (not yet constructed!) HashSet >+ * will return 'false' for initialized(). > */ ... >diff --git a/js/src/jsatom.h b/js/src/jsatom.h +struct JSAtomState +{ + /* We cannot rely on constructors/destructors, so do it manually. */ + js::AlignedStorage<sizeof(js::AtomSet)> atomStorage; JSRuntime that contains AtomState is properly constrcuted, see JS_NewRuntime/JS_DestroyRuntime in jsapi.cpp. So I do not see why it is necessary to call constructors and destructors explicitly. So either rely on them or explain in comments the exact reason. ... >+typedef size_t AtomEntryType; Nit: use uintptr_t, not size_t, here. r+ with this addressed.

(In reply to comment #2) > JSRuntime that contains AtomState is properly constrcuted, see > JS_NewRuntime/JS_DestroyRuntime in jsapi.cpp. \o/

Backed out

(In reply to comment #4) > http://hg.mozilla.org/tracemonkey/rev/89dc3a238da0 Nit: remove /* We cannot rely on constructors/destructors, so do it manually. */ from JSAtomState definition in jsatom.h

(In reply to comment #2) > >+static JS_ALWAYS_INLINE void > >+ClearAtomEntryFlags(const AtomEntryType &entry, uintN flags) > > { > >+ const_cast<AtomEntryType &>(entry) &= ~(uintN)flags; > > Nit: flags is uintN so no need to cast. I was wrong here as the cast is necessary but that should be a constructor call for AtomEntryType like in ~AtomEntryType(flags). Otherwise ~flags are zero-expanded on 64 bits. Also add an assert that flags <= ATOM_ENTRY_FLAG_MASK.

(In reply to comment #7) > > Nit: flags is uintN so no need to cast. > > I was wrong here as the cast is necessary but that should be a constructor call > for AtomEntryType like in ~AtomEntryType(flags). Otherwise ~flags are > zero-expanded on 64 bits. Also add an assert that flags <= > ATOM_ENTRY_FLAG_MASK. Hah, all this time I've assumed uintN was yet another size_t (where N = sizeof(void*)), and just used size_t instead. Thanks!