Bug 802318 - Remove the invalid SkipRoot in AtomizeInline; r=billm

This re-organizes AtomizeInline to separate the TakeOwnership and Copy cases.

--HG--
extra : rebase_source : 2045f8503e7ff0419f992e4268683d1b63d5f094

js/src/jsatom.cpp

@@ -236,67 +236,100 @@ enum OwnCharsBehavior
 };
 
 /*
- * Callers passing OwnChars have freshly allocated *pchars and thus this
+ * When the jschars reside in a freshly allocated buffer the memory can be used
- * memory can be used as a new JSAtom's buffer without copying. When this flag
+ * as a new JSAtom's storage without copying. The contract is that the caller no
- * is set, the contract is that callers will free *pchars iff *pchars == NULL.
+ * longer owns the memory and this method is responsible for freeing the memory.
  */
 JS_ALWAYS_INLINE
-static JSAtom *
+static UnrootedAtom
-AtomizeInline(JSContext *cx, const jschar **pchars, size_t length,
+AtomizeAndTakeOwnership(JSContext *cx, StableCharPtr tbchars, size_t length,
-              InternBehavior ib, OwnCharsBehavior ocb = CopyChars)
+                           InternBehavior ib)
 {
-    const jschar *chars = *pchars;
+    JS_ASSERT(tbchars[length] == 0);
 
-    if (JSAtom *s = cx->runtime->staticStrings.lookup(chars, length))
+    if (UnrootedAtom s = cx->runtime->staticStrings.lookup(tbchars.get(), length)) {
+        js_free((void*)tbchars.get());
         return s;
+    }
 
-    AtomSet &atoms = cx->runtime->atoms;
+    /*
-    AtomSet::AddPtr p = atoms.lookupForAdd(AtomHasher::Lookup(chars, length));
+     * If a GC occurs at js_NewStringCopy then |p| will still have the correct
-
+     * hash, allowing us to avoid rehashing it. Even though the hash is
+     * unchanged, we need to re-lookup the table position because a last-ditch
+     * GC will potentially free some table entries.
+     */
+    AtomHasher::Lookup lookup(tbchars.get(), length);
+    AtomSet::AddPtr p = cx->runtime->atoms.lookupForAdd(lookup);
+    SkipRoot skipHash(cx, &p); /* Prevent the hash from being poisoned. */
     if (p) {
-        JSAtom *atom = p->asPtr();
+        UnrootedAtom atom = p->asPtr();
+        p->setTagged(bool(ib));
+        js_free((void*)tbchars.get());
+        return atom;
+    }
+
+    AutoEnterAtomsCompartment ac(cx);
+
+    UnrootedFlatString flat = js_NewString(cx, const_cast<jschar*>(tbchars.get()), length);
+    if (!flat) {
+        js_free((void*)tbchars.get());
+        return UnrootedAtom();
+    }
+
+    UnrootedAtom atom = flat->morphAtomizedStringIntoAtom();
+
+    if (!cx->runtime->atoms.relookupOrAdd(p, lookup, AtomStateEntry(atom, bool(ib)))) {
+        JS_ReportOutOfMemory(cx); /* SystemAllocPolicy does not report OOM. */
+        return UnrootedAtom();
+    }
+
+    return atom;
+}
+
+/* |tbchars| must not point into an inline or short string. */
+JS_ALWAYS_INLINE
+static UnrootedAtom
+AtomizeAndCopyStableChars(JSContext *cx, const jschar *tbchars, size_t length, InternBehavior ib)
+{
+    if (UnrootedAtom s = cx->runtime->staticStrings.lookup(tbchars, length))
+         return s;
+
+    /*
+     * If a GC occurs at js_NewStringCopy then |p| will still have the correct
+     * hash, allowing us to avoid rehashing it. Even though the hash is
+     * unchanged, we need to re-lookup the table position because a last-ditch
+     * GC will potentially free some table entries.
+     */
+    AtomHasher::Lookup lookup(tbchars, length);
+    AtomSet::AddPtr p = cx->runtime->atoms.lookupForAdd(lookup);
+    SkipRoot skipHash(cx, &p); /* Prevent the hash from being poisoned. */
+    if (p) {
+        UnrootedAtom atom = p->asPtr();
         p->setTagged(bool(ib));
         return atom;
     }
 
     AutoEnterAtomsCompartment ac(cx);
 
-    SkipRoot skip(cx, &chars);
+    UnrootedFlatString flat = js_NewStringCopyN(cx, tbchars, length);
+    if (!flat)
+        return UnrootedAtom();
 
-    /* Workaround for hash values in AddPtr being inadvertently poisoned. */
+    UnrootedAtom atom = flat->morphAtomizedStringIntoAtom();
-    SkipRoot skip2(cx, &p);
 
-    JSFlatString *key;
+    if (!cx->runtime->atoms.relookupOrAdd(p, lookup, AtomStateEntry(atom, bool(ib)))) {
-    if (ocb == TakeCharOwnership) {
+        JS_ReportOutOfMemory(cx); /* SystemAllocPolicy does not report OOM. */
-        key = js_NewString(cx, const_cast<jschar *>(chars), length);
+        return UnrootedAtom();
-        *pchars = NULL; /* Called should not free *pchars. */
-    } else {
-        JS_ASSERT(ocb == CopyChars);
-        key = js_NewStringCopyN(cx, chars, length);
-    }
-    if (!key)
-        return NULL;
-
-    /*
-     * We have to relookup the key as the last ditch GC invoked from the
-     * string allocation or OOM handling unlocks the atomsCompartment.
-     *
-     * N.B. this avoids recomputing the hash but still has a potential
-     * (# collisions * # chars) comparison cost in the case of a hash
-     * collision!
-     */
-    AtomHasher::Lookup lookup(chars, length);
-    if (!atoms.relookupOrAdd(p, lookup, AtomStateEntry((JSAtom *) key, bool(ib)))) {
-        JS_ReportOutOfMemory(cx); /* SystemAllocPolicy does not report */
-        return NULL;
     }
 
-    return key->morphAtomizedStringIntoAtom();
+    return atom;
 }
 
-JSAtom *
+UnrootedAtom
-js::AtomizeString(JSContext *cx, JSString *str, InternBehavior ib)
+js::AtomizeString(JSContext *cx, JSString *str, js::InternBehavior ib /* = js::DoNotInternAtom */)
 {
+    AssertCanGC();
+
     if (str->isAtom()) {
         JSAtom &atom = str->asAtom();
         /* N.B. static atoms are effectively always interned. */
@@ -315,60 +348,53 @@ js::AtomizeString(JSContext *cx, JSString *str, InternBehavior ib)
     if (!stable)
         return NULL;
 
-    const jschar *chars = stable->chars().get();
+    JS_ASSERT(stable->length() <= JSString::MAX_LENGTH);
-    size_t length = stable->length();
+    return AtomizeAndCopyStableChars(cx, stable->chars().get(), stable->length(), ib);
-    JS_ASSERT(length <= JSString::MAX_LENGTH);
-    return AtomizeInline(cx, &chars, length, ib);
 }
 
-JSAtom *
+UnrootedAtom
 js::Atomize(JSContext *cx, const char *bytes, size_t length, InternBehavior ib)
 {
+    AssertCanGC();
     CHECK_REQUEST(cx);
 
     if (!JSString::validateLength(cx, length))
         return NULL;
 
-    /*
+    UnrootedAtom atom;
-     * Avoiding the malloc in InflateString on shorter strings saves us
-     * over 20,000 malloc calls on mozilla browser startup. This compares to
-     * only 131 calls where the string is longer than a 31 char (net) buffer.
-     * The vast majority of atomized strings are already in the hashtable. So
-     * js::AtomizeString rarely has to copy the temp string we make.
-     */
     static const unsigned ATOMIZE_BUF_MAX = 32;
-    jschar inflated[ATOMIZE_BUF_MAX];
-    size_t inflatedLength = ATOMIZE_BUF_MAX - 1;
-
-    const jschar *chars;
-    OwnCharsBehavior ocb = CopyChars;
     if (length < ATOMIZE_BUF_MAX) {
+        /*
+         * Avoiding the malloc in InflateString on shorter strings saves us
+         * over 20,000 malloc calls on mozilla browser startup. This compares to
+         * only 131 calls where the string is longer than a 31 char (net) buffer.
+         * The vast majority of atomized strings are already in the hashtable. So
+         * js::AtomizeString rarely has to copy the temp string we make.
+         */
+        jschar inflated[ATOMIZE_BUF_MAX];
+        size_t inflatedLength = ATOMIZE_BUF_MAX - 1;
         InflateStringToBuffer(cx, bytes, length, inflated, &inflatedLength);
-        inflated[inflatedLength] = 0;
+        atom = AtomizeAndCopyStableChars(cx, inflated, inflatedLength, ib);
-        chars = inflated;
     } else {
-        inflatedLength = length;
+        jschar *tbcharsZ = InflateString(cx, bytes, &length);
-        chars = InflateString(cx, bytes, &inflatedLength);
+        if (!tbcharsZ)
-        if (!chars)
+            return UnrootedAtom();
-            return NULL;
+        atom = AtomizeAndTakeOwnership(cx, StableCharPtr(tbcharsZ, length), length, ib);
-        ocb = TakeCharOwnership;
     }
 
-    JSAtom *atom = AtomizeInline(cx, &chars, inflatedLength, ib, ocb);
-    if (ocb == TakeCharOwnership && chars)
-        js_free((void *)chars);
     return atom;
 }
 
-JSAtom *
+UnrootedAtom
 js::AtomizeChars(JSContext *cx, const jschar *chars, size_t length, InternBehavior ib)
 {
+    AssertCanGC();
     CHECK_REQUEST(cx);
 
     if (!JSString::validateLength(cx, length))
         return NULL;
 
-    return AtomizeInline(cx, &chars, length, ib);
+    return AtomizeAndCopyStableChars(cx, chars, length, ib);
 }
 
 bool

js/src/jsatom.h

@@ -23,6 +23,8 @@
 #include "js/HashTable.h"
 #include "vm/CommonPropertyNames.h"
 
+ForwardDeclareJS(Atom);
+
 struct JSIdArray {
     int length;
     js::HeapId vector[1];    /* actually, length jsid words */
@@ -79,7 +81,7 @@ class AtomStateEntry
   public:
     AtomStateEntry() : bits(0) {}
     AtomStateEntry(const AtomStateEntry &other) : bits(other.bits) {}
-    AtomStateEntry(JSAtom *ptr, bool tagged)
+    AtomStateEntry(RawAtom ptr, bool tagged)
       : bits(uintptr_t(ptr) | uintptr_t(tagged))
     {
         JS_ASSERT((uintptr_t(ptr) & 0x1) == 0);
@@ -220,15 +222,15 @@ enum InternBehavior
     InternAtom = true
 };
 
-extern JSAtom *
+extern UnrootedAtom
 Atomize(JSContext *cx, const char *bytes, size_t length,
         js::InternBehavior ib = js::DoNotInternAtom);
 
-extern JSAtom *
+extern UnrootedAtom
 AtomizeChars(JSContext *cx, const jschar *chars, size_t length,
              js::InternBehavior ib = js::DoNotInternAtom);
 
-extern JSAtom *
+extern UnrootedAtom
 AtomizeString(JSContext *cx, JSString *str, js::InternBehavior ib = js::DoNotInternAtom);
 
 inline JSAtom *

js/src/vm/String.h

@@ -28,7 +28,8 @@ class JSLinearString;
 class JSStableString;
 class JSInlineString;
 class JSRope;
-class JSAtom;
+ForwardDeclareJS(FlatString);
+ForwardDeclareJS(Atom);
 
 namespace js {