Bug 1449541 - Don't hold script data lock while calcuating hash r=jandem
authorJon Coppeard <jcoppeard@mozilla.com>
Wed, 28 Mar 2018 16:41:16 +0100
changeset 410491 4be93a2b5a928ae3a613cf8378f12e4ba88afa85
parent 410490 9be3b370ae62e248626b62f3bb8d2af941423cab
child 410492 87094362a159e708117f3f45ee64945078aa7342
push id33729
push userrgurzau@mozilla.com
push dateWed, 28 Mar 2018 21:55:49 +0000
treeherdermozilla-central@6aa3b57955fe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1449541
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 1449541 - Don't hold script data lock while calcuating hash r=jandem
js/src/vm/JSScript.cpp
js/src/vm/JSScript.h
--- a/js/src/vm/JSScript.cpp
+++ b/js/src/vm/JSScript.cpp
@@ -2377,50 +2377,62 @@ js::SharedScriptData::new_(JSContext* cx
      * We expect bytecode is always non-empty. */
     MOZ_DIAGNOSTIC_ASSERT(codeLength > 0);
 
     entry->refCount_ = 0;
     entry->natoms_ = natoms;
     entry->codeLength_ = codeLength;
     entry->noteLength_ = srcnotesLength;
 
-
     /*
      * Call constructors to initialize the storage that will be accessed as a
      * GCPtrAtom array via atoms().
      */
     static_assert(offsetof(SharedScriptData, data_) % sizeof(GCPtrAtom) == 0,
                   "atoms must have GCPtrAtom alignment");
     GCPtrAtom* atoms = entry->atoms();
     for (unsigned i = 0; i < natoms; ++i)
         new (&atoms[i]) GCPtrAtom();
 
     // Sanity check the dataLength() computation
     MOZ_ASSERT(entry->dataLength() == dataLength);
 
     return entry;
 }
 
+inline
+js::ScriptBytecodeHasher::Lookup::Lookup(SharedScriptData* data)
+  : scriptData(data),
+    hash(mozilla::HashBytes(scriptData->data(), scriptData->dataLength()))
+{
+    scriptData->incRefCount();
+}
+
+inline
+js::ScriptBytecodeHasher::Lookup::~Lookup()
+{
+    scriptData->decRefCount();
+}
+
 bool
 JSScript::createScriptData(JSContext* cx, uint32_t codeLength, uint32_t srcnotesLength,
                            uint32_t natoms)
 {
     MOZ_ASSERT(!scriptData());
     SharedScriptData* ssd = SharedScriptData::new_(cx, codeLength, srcnotesLength, natoms);
     if (!ssd)
         return false;
 
     setScriptData(ssd);
     return true;
 }
 
 void
 JSScript::freeScriptData()
 {
-    MOZ_ASSERT(scriptData_->refCount() == 1);
     scriptData_->decRefCount();
     scriptData_ = nullptr;
 }
 
 void
 JSScript::setScriptData(js::SharedScriptData* data)
 {
     MOZ_ASSERT(!scriptData_);
@@ -2436,19 +2448,23 @@ JSScript::setScriptData(js::SharedScript
  */
 bool
 JSScript::shareScriptData(JSContext* cx)
 {
     SharedScriptData* ssd = scriptData();
     MOZ_ASSERT(ssd);
     MOZ_ASSERT(ssd->refCount() == 1);
 
+    // Calculate the hash before taking the lock. Because the data is reference
+    // counted, it also will be freed after releasing the lock if necessary.
+    ScriptBytecodeHasher::Lookup lookup(ssd);
+
     AutoLockScriptData lock(cx->runtime());
 
-    ScriptDataTable::AddPtr p = cx->scriptDataTable(lock).lookupForAdd(*ssd);
+    ScriptDataTable::AddPtr p = cx->scriptDataTable(lock).lookupForAdd(lookup);
     if (p) {
         MOZ_ASSERT(ssd != *p);
         freeScriptData();
         setScriptData(*p);
     } else {
         if (!cx->scriptDataTable(lock).add(p, ssd)) {
             freeScriptData();
             ReportOutOfMemory(cx);
--- a/js/src/vm/JSScript.h
+++ b/js/src/vm/JSScript.h
@@ -851,29 +851,39 @@ class SharedScriptData
   private:
     SharedScriptData() = delete;
     SharedScriptData(const SharedScriptData&) = delete;
     SharedScriptData& operator=(const SharedScriptData&) = delete;
 };
 
 struct ScriptBytecodeHasher
 {
-    typedef SharedScriptData Lookup;
+    class Lookup {
+        friend struct ScriptBytecodeHasher;
+
+        SharedScriptData* scriptData;
+        HashNumber hash;
+
+      public:
+        explicit Lookup(SharedScriptData* data);
+        ~Lookup();
+    };
 
     static HashNumber hash(const Lookup& l) {
-        return mozilla::HashBytes(l.data(), l.dataLength());
+        return l.hash;
     }
     static bool match(SharedScriptData* entry, const Lookup& lookup) {
-        if (entry->natoms() != lookup.natoms())
-            return false;
-        if (entry->codeLength() != lookup.codeLength())
+        const SharedScriptData* data = lookup.scriptData;
+        if (entry->natoms() != data->natoms())
             return false;
-        if (entry->numNotes() != lookup.numNotes())
+        if (entry->codeLength() != data->codeLength())
             return false;
-        return mozilla::PodEqual<uint8_t>(entry->data(), lookup.data(), lookup.dataLength());
+        if (entry->numNotes() != data->numNotes())
+            return false;
+        return mozilla::PodEqual<uint8_t>(entry->data(), data->data(), data->dataLength());
     }
 };
 
 class AutoLockScriptData;
 
 using ScriptDataTable = HashSet<SharedScriptData*,
                                 ScriptBytecodeHasher,
                                 SystemAllocPolicy>;