Bug 1445196 - Store the atom's pinned flag in the atom to simplify lookup r=jandem
authorJon Coppeard <jcoppeard@mozilla.com>
Tue, 13 Mar 2018 18:36:47 +0000
changeset 461566 471a52bfd1eedc639c7c814bc177c44134867065
parent 461565 3af13e1ae15a66c1956de5476953f32e1d77c0d3
child 461567 e18f1f40d080a35be5ad07a9647489aa8bad3e71
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1445196
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1445196 - Store the atom's pinned flag in the atom to simplify lookup r=jandem
js/src/gc/AtomMarking-inl.h
js/src/gc/AtomMarking.cpp
js/src/jsapi.cpp
js/src/vm/HelperThreads.cpp
js/src/vm/JSAtom.cpp
js/src/vm/JSAtom.h
js/src/vm/StringType.cpp
js/src/vm/StringType.h
--- a/js/src/gc/AtomMarking-inl.h
+++ b/js/src/gc/AtomMarking-inl.h
@@ -20,17 +20,17 @@ GetAtomBit(TenuredCell* thing)
     Arena* arena = thing->arena();
     size_t arenaBit = (reinterpret_cast<uintptr_t>(thing) - arena->address()) / CellBytesPerMarkBit;
     return arena->atomBitmapStart() * JS_BITS_PER_WORD + arenaBit;
 }
 
 inline bool
 ThingIsPermanent(JSAtom* atom)
 {
-    return atom->isPermanentAtom();
+    return atom->isPinned();
 }
 
 inline bool
 ThingIsPermanent(JS::Symbol* symbol)
 {
     return symbol->isWellKnownSymbol();
 }
 
--- a/js/src/gc/AtomMarking.cpp
+++ b/js/src/gc/AtomMarking.cpp
@@ -213,22 +213,16 @@ AtomMarkingRuntime::atomIsMarked(Zone* z
     MOZ_ASSERT(thing->zoneFromAnyThread()->isAtomsZone());
 
     if (!zone->runtimeFromAnyThread()->permanentAtoms)
         return true;
 
     if (ThingIsPermanent(thing))
         return true;
 
-    if (mozilla::IsSame<T, JSAtom>::value) {
-        JSAtom* atom = reinterpret_cast<JSAtom*>(thing);
-        if (AtomIsPinnedInRuntime(zone->runtimeFromAnyThread(), atom))
-            return true;
-    }
-
     size_t bit = GetAtomBit(&thing->asTenured());
     return zone->markedAtoms().getBit(bit);
 }
 
 template bool AtomMarkingRuntime::atomIsMarked(Zone* zone, JSAtom* thing);
 template bool AtomMarkingRuntime::atomIsMarked(Zone* zone, JS::Symbol* thing);
 
 template<>
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -5808,17 +5808,17 @@ JS_PUBLIC_API(bool)
 JS_StringHasBeenPinned(JSContext* cx, JSString* str)
 {
     AssertHeapIsIdle();
     CHECK_REQUEST(cx);
 
     if (!str->isAtom())
         return false;
 
-    return AtomIsPinned(cx, &str->asAtom());
+    return str->asAtom().isPinned();
 }
 
 JS_PUBLIC_API(jsid)
 INTERNED_STRING_TO_JSID(JSContext* cx, JSString* str)
 {
     MOZ_ASSERT(str);
     MOZ_ASSERT(((size_t)str & JSID_TYPE_MASK) == 0);
     MOZ_ASSERT_IF(cx, JS_StringHasBeenPinned(cx, str));
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -2077,24 +2077,16 @@ js::StartOffThreadPromiseHelperTask(Prom
 
     HelperThreadState().notifyOne(GlobalHelperThreadState::PRODUCER, lock);
     return true;
 }
 
 void
 GlobalHelperThreadState::trace(JSTracer* trc, gc::AutoTraceSession& session)
 {
-    // There's an assertion that requires the exclusive access lock when tracing
-    // atoms (see AtomIsPinnedInRuntime). Due to mutex ordering requirements we
-    // need to take that lock before the helper thread lock, if we don't have it
-    // already.
-    Maybe<AutoLockForExclusiveAccess> exclusiveLock;
-    if (!session.maybeLock.isSome())
-        exclusiveLock.emplace(trc->runtime());
-
     AutoLockHelperThreadState lock;
     for (auto builder : ionWorklist(lock))
         builder->trace(trc);
     for (auto builder : ionFinishedList(lock))
         builder->trace(trc);
 
     if (HelperThreadState().threads) {
         for (auto& helper : *HelperThreadState().threads) {
--- a/js/src/vm/JSAtom.cpp
+++ b/js/src/vm/JSAtom.cpp
@@ -245,16 +245,17 @@ JSRuntime::finishAtoms()
     emptyString = nullptr;
 }
 
 static inline void
 TracePinnedAtoms(JSTracer* trc, const AtomSet& atoms)
 {
     for (auto r = atoms.all(); !r.empty(); r.popFront()) {
         const AtomStateEntry& entry = r.front();
+        MOZ_ASSERT(entry.isPinned() == entry.asPtrUnbarriered()->isPinned());
         if (entry.isPinned()) {
             JSAtom* atom = entry.asPtrUnbarriered();
             TraceRoot(trc, &atom, "interned_atom");
             MOZ_ASSERT(entry.asPtrUnbarriered() == atom);
         }
     }
 }
 
@@ -284,17 +285,18 @@ js::TracePermanentAtoms(JSTracer* trc)
     if (rt->staticStrings)
         rt->staticStrings->trace(trc);
 
     if (rt->permanentAtoms) {
         for (FrozenAtomSet::Range r(rt->permanentAtoms->all()); !r.empty(); r.popFront()) {
             const AtomStateEntry& entry = r.front();
 
             JSAtom* atom = entry.asPtrUnbarriered();
-            TraceProcessGlobalRoot(trc, atom, "permanent_table");
+            MOZ_ASSERT(atom->isPinned());
+            TraceProcessGlobalRoot(trc, atom, "permanent atom");
         }
     }
 }
 
 void
 js::TraceWellKnownSymbols(JSTracer* trc)
 {
     JSRuntime* rt = trc->runtime();
@@ -338,59 +340,16 @@ LookupAtomState(JSRuntime* rt, const Ato
     MOZ_ASSERT(rt->currentThreadHasExclusiveAccess());
 
     AtomSet::Ptr p = rt->unsafeAtoms().lookup(lookup); // Safe because we hold the lock.
     if (!p && rt->atomsAddedWhileSweeping())
         p = rt->atomsAddedWhileSweeping()->lookup(lookup);
     return p;
 }
 
-bool
-AtomIsPinned(JSContext* cx, JSAtom* atom)
-{
-    /* We treat static strings as interned because they're never collected. */
-    if (StaticStrings::isStatic(atom))
-        return true;
-
-    AtomHasher::Lookup lookup(atom);
-
-    /* Likewise, permanent strings are considered to be interned. */
-    MOZ_ASSERT(cx->isPermanentAtomsInitialized());
-    AtomSet::Ptr p = cx->permanentAtoms().readonlyThreadsafeLookup(lookup);
-    if (p)
-        return true;
-
-    AutoLockForExclusiveAccess lock(cx);
-
-    p = LookupAtomState(cx->runtime(), lookup);
-    if (!p)
-        return false;
-
-    return p->isPinned();
-}
-
-#ifdef DEBUG
-
-bool
-AtomIsPinnedInRuntime(JSRuntime* rt, JSAtom* atom)
-{
-    Maybe<AutoLockForExclusiveAccess> lock;
-    if (!rt->currentThreadHasExclusiveAccess())
-        lock.emplace(rt);
-
-    AtomHasher::Lookup lookup(atom);
-
-    AtomSet::Ptr p = LookupAtomState(rt, lookup);
-    MOZ_ASSERT(p);
-
-    return p->isPinned();
-}
-
-#endif // DEBUG
-
 template <typename CharT>
 MOZ_ALWAYS_INLINE
 static JSAtom*
 AtomizeAndCopyCharsInner(JSContext* cx, const CharT* tbchars, size_t length, PinningBehavior pin,
                          const Maybe<uint32_t>& indexValue, const AtomHasher::Lookup& lookup);
 
 /* |tbchars| must not point into an inline or short string. */
 template <typename CharT>
@@ -483,17 +442,20 @@ AtomizeAndCopyCharsInner(JSContext* cx, 
                 if (!IsAboutToBeFinalizedUnbarriered(&atom))
                     p = p2;
             }
         }
     }
 
     if (p) {
         JSAtom* atom = p->asPtr(cx);
-        p->setPinned(bool(pin));
+        if (pin && !atom->isPinned()) {
+            atom->setPinned();
+            p->setPinned(true);
+        }
         return atom;
     }
 
     JSAtom* atom;
     {
         AutoAtomsCompartment ac(cx, lock);
 
         JSFlatString* flat = NewStringCopyN<NoGC>(cx, tbchars, length);
@@ -503,16 +465,19 @@ AtomizeAndCopyCharsInner(JSContext* cx, 
             // please also fix or comment the similar case in Symbol::new_.
             ReportOutOfMemory(cx);
             return nullptr;
         }
 
         atom = flat->morphAtomizedStringIntoAtom(lookup.hash);
         MOZ_ASSERT(atom->hash() == lookup.hash);
 
+        if (pin)
+            atom->setPinned();
+
         if (indexValue)
             atom->maybeInitializeIndex(*indexValue, true);
 
         // We have held the lock since looking up p, and the operations we've done
         // since then can't GC; therefore the atoms table has not been modified and
         // p is still valid.
         AtomSet* addSet = atomsAddedWhileSweeping ? atomsAddedWhileSweeping : &atoms;
         if (!addSet->add(p, AtomStateEntry(atom, bool(pin)))) {
@@ -534,34 +499,31 @@ AtomizeAndCopyChars(JSContext* cx, const
 
 JSAtom*
 js::AtomizeString(JSContext* cx, JSString* str,
                   js::PinningBehavior pin /* = js::DoNotPinAtom */)
 {
     if (str->isAtom()) {
         JSAtom& atom = str->asAtom();
         /* N.B. static atoms are effectively always interned. */
-        if (pin != PinAtom || js::StaticStrings::isStatic(&atom))
+        if (pin != PinAtom || atom.isPinned())
             return &atom;
 
         AtomHasher::Lookup lookup(&atom);
 
-        /* Likewise, permanent atoms are always interned. */
-        MOZ_ASSERT(cx->isPermanentAtomsInitialized());
-        AtomSet::Ptr p = cx->permanentAtoms().readonlyThreadsafeLookup(lookup);
-        if (p)
-            return &atom;
-
         AutoLockForExclusiveAccess lock(cx);
 
-        p = LookupAtomState(cx->runtime(), lookup);
-        MOZ_ASSERT(p); /* Non-static atom must exist in atom state set. */
+        AtomSet::Ptr p = LookupAtomState(cx->runtime(), lookup);
+        MOZ_ASSERT(p); // Unpinned atoms must exist in atoms table.
         MOZ_ASSERT(p->asPtrUnbarriered() == &atom);
+
         MOZ_ASSERT(pin == PinAtom);
-        p->setPinned(bool(pin));
+        atom.setPinned();
+        p->setPinned(true);
+
         return &atom;
     }
 
     JSLinearString* linear = str->ensureLinear(cx);
     if (!linear)
         return nullptr;
 
     Maybe<uint32_t> indexValue;
--- a/js/src/vm/JSAtom.h
+++ b/js/src/vm/JSAtom.h
@@ -24,27 +24,16 @@ namespace js {
  */
 extern const char*
 AtomToPrintableString(JSContext* cx, JSAtom* atom, JSAutoByteString* bytes);
 
 class PropertyName;
 
 }  /* namespace js */
 
-extern bool
-AtomIsPinned(JSContext* cx, JSAtom* atom);
-
-#ifdef DEBUG
-
-// This may be called either with or without the atoms lock held.
-extern bool
-AtomIsPinnedInRuntime(JSRuntime* rt, JSAtom* atom);
-
-#endif // DEBUG
-
 /* Well-known predefined C strings. */
 #define DECLARE_PROTO_STR(name,init,clasp) extern const char js_##name##_str[];
 JS_FOR_EACH_PROTOTYPE(DECLARE_PROTO_STR)
 #undef DECLARE_PROTO_STR
 
 #define DECLARE_CONST_CHAR_STR(idpart, id, text)  extern const char js_##idpart##_str[];
 FOR_EACH_COMMON_PROPERTYNAME(DECLARE_CONST_CHAR_STR)
 #undef DECLARE_CONST_CHAR_STR
--- a/js/src/vm/StringType.cpp
+++ b/js/src/vm/StringType.cpp
@@ -1062,30 +1062,37 @@ StaticStrings::init(JSContext* cx)
 
         // Static string initialization can not race, so allow even without the lock.
         intStaticTable[i]->maybeInitializeIndex(i, true);
     }
 
     return true;
 }
 
+inline void
+TraceStaticString(JSTracer* trc, JSAtom* atom, const char* name)
+{
+    MOZ_ASSERT(atom->isPinned());
+    TraceProcessGlobalRoot(trc, atom, name);
+}
+
 void
 StaticStrings::trace(JSTracer* trc)
 {
     /* These strings never change, so barriers are not needed. */
 
     for (uint32_t i = 0; i < UNIT_STATIC_LIMIT; i++)
-        TraceProcessGlobalRoot(trc, unitStaticTable[i], "unit-static-string");
+        TraceStaticString(trc, unitStaticTable[i], "unit-static-string");
 
     for (uint32_t i = 0; i < NUM_SMALL_CHARS * NUM_SMALL_CHARS; i++)
-        TraceProcessGlobalRoot(trc, length2StaticTable[i], "length2-static-string");
+        TraceStaticString(trc, length2StaticTable[i], "length2-static-string");
 
     /* This may mark some strings more than once, but so be it. */
     for (uint32_t i = 0; i < INT_STATIC_LIMIT; i++)
-        TraceProcessGlobalRoot(trc, intStaticTable[i], "int-static-string");
+        TraceStaticString(trc, intStaticTable[i], "int-static-string");
 }
 
 template <typename CharT>
 /* static */ bool
 StaticStrings::isStatic(const CharT* chars, size_t length)
 {
     switch (length) {
       case 1: {
--- a/js/src/vm/StringType.h
+++ b/js/src/vm/StringType.h
@@ -285,16 +285,18 @@ class JSString : public js::gc::Cell
 
     static const uint32_t TYPE_FLAGS_MASK        = JS_BIT(6) - 1;
 
     static const uint32_t LATIN1_CHARS_BIT       = JS_BIT(6);
 
     static const uint32_t INDEX_VALUE_BIT        = JS_BIT(7);
     static const uint32_t INDEX_VALUE_SHIFT      = 16;
 
+    static const uint32_t PINNED_ATOM_BIT        = JS_BIT(8);
+
     static const uint32_t MAX_LENGTH             = js::MaxStringLength;
 
     static const JS::Latin1Char MAX_LATIN1_CHAR = 0xff;
 
     /*
      * Helper function to validate that a string of a given length is
      * representable by a JSString. An allocation overflow is reported if false
      * is returned.
@@ -1151,20 +1153,32 @@ class JSAtom : public JSFlatString
     inline void finalize(js::FreeOp* fop);
 
     MOZ_ALWAYS_INLINE
     bool isPermanent() const {
         return JSString::isPermanentAtom();
     }
 
     // Transform this atom into a permanent atom. This is only done during
-    // initialization of the runtime.
+    // initialization of the runtime. Permanent atoms are always pinned.
     MOZ_ALWAYS_INLINE void morphIntoPermanentAtom() {
         MOZ_ASSERT(static_cast<JSString*>(this)->isAtom());
-        d.u1.flags |= PERMANENT_ATOM_FLAGS;
+        d.u1.flags |= PERMANENT_ATOM_FLAGS | PINNED_ATOM_BIT;
+    }
+
+    MOZ_ALWAYS_INLINE
+    bool isPinned() const {
+        return d.u1.flags & PINNED_ATOM_BIT;
+    }
+
+    // Mark the atom as pinned. For use by atomization only.
+    MOZ_ALWAYS_INLINE void setPinned() {
+        MOZ_ASSERT(static_cast<JSString*>(this)->isAtom());
+        MOZ_ASSERT(!isPinned());
+        d.u1.flags |= PINNED_ATOM_BIT;
     }
 
     inline js::HashNumber hash() const;
     inline void initHash(js::HashNumber hash);
 
 #ifdef DEBUG
     void dump(js::GenericPrinter& out);
     void dump();
@@ -1244,17 +1258,17 @@ JSFlatString::morphAtomizedStringIntoAto
     atom->initHash(hash);
     return atom;
 }
 
 MOZ_ALWAYS_INLINE JSAtom*
 JSFlatString::morphAtomizedStringIntoPermanentAtom(js::HashNumber hash)
 {
     MOZ_ASSERT(!isAtom());
-    d.u1.flags |= PERMANENT_ATOM_FLAGS;
+    d.u1.flags |= PERMANENT_ATOM_FLAGS | PINNED_ATOM_BIT;
     d.u1.flags &= ~NON_ATOM_BIT;
     JSAtom* atom = &asAtom();
     atom->initHash(hash);
     return atom;
 }
 
 namespace js {