Bug 1533755 - Use RefPtr for SharedScriptData pointers. r=jandem
☠☠ backed out by 638b7cfb8639 ☠ ☠
authorTed Campbell <tcampbell@mozilla.com>
Mon, 11 Mar 2019 20:01:08 +0000
changeset 521424 d5eff5c34ce9c47055b6599487c837cc6363d6b5
parent 521423 35825a8005ab0f60ec24b2655e771d090f6c36d6
child 521425 638b7cfb8639eb6ba386696940a5225adce33100
push id10866
push usernerli@mozilla.com
push dateTue, 12 Mar 2019 18:59:09 +0000
treeherdermozilla-beta@445c24a51727 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1533755
milestone67.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 1533755 - Use RefPtr for SharedScriptData pointers. r=jandem Leave manual refcounting in the ScriptDataTable for now since it requires a bit of care to make the automatic types do the right thing when sweeping. Differential Revision: https://phabricator.services.mozilla.com/D22718
js/src/vm/JSScript.cpp
js/src/vm/JSScript.h
--- a/js/src/vm/JSScript.cpp
+++ b/js/src/vm/JSScript.cpp
@@ -2903,49 +2903,26 @@ SharedScriptData* js::SharedScriptData::
   // 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();
-}
+      hash(mozilla::HashBytes(scriptData->data(), scriptData->dataLength())) {}
 
 bool JSScript::createSharedScriptData(JSContext* cx, uint32_t codeLength,
                                       uint32_t noteLength, uint32_t natoms) {
-  MOZ_ASSERT(!scriptData());
-  SharedScriptData* ssd =
-      SharedScriptData::new_(cx, codeLength, noteLength, natoms);
-  if (!ssd) {
-    return false;
-  }
-
-  setScriptData(ssd);
-  return true;
+  MOZ_ASSERT(!scriptData_);
+  scriptData_ = SharedScriptData::new_(cx, codeLength, noteLength, natoms);
+  return !!scriptData_;
 }
 
-void JSScript::freeScriptData() {
-  if (scriptData_) {
-    scriptData_->decRefCount();
-    scriptData_ = nullptr;
-  }
-}
-
-void JSScript::setScriptData(js::SharedScriptData* data) {
-  MOZ_ASSERT(!scriptData_);
-  scriptData_ = data;
-  scriptData_->incRefCount();
-}
+void JSScript::freeScriptData() { scriptData_ = nullptr; }
 
 /*
  * Takes ownership of its *ssd parameter and either adds it into the runtime's
  * ScriptDataTable or frees it if a matching entry already exists.
  *
  * Sets the |code| and |atoms| fields on the given JSScript.
  */
 bool JSScript::shareScriptData(JSContext* cx) {
@@ -2957,44 +2934,44 @@ bool JSScript::shareScriptData(JSContext
   // 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(lookup);
   if (p) {
     MOZ_ASSERT(ssd != *p);
-    freeScriptData();
-    setScriptData(*p);
+    scriptData_ = *p;
   } else {
     if (!cx->scriptDataTable(lock).add(p, ssd)) {
-      freeScriptData();
       ReportOutOfMemory(cx);
       return false;
     }
 
     // Being in the table counts as a reference on the script data.
-    scriptData()->incRefCount();
-  }
-
-  MOZ_ASSERT(scriptData()->refCount() >= 2);
+    ssd->AddRef();
+  }
+
+  // Refs: JSScript, Lookup, ScriptDataTable
+  MOZ_ASSERT(scriptData()->refCount() >= 3);
+
   return true;
 }
 
 void js::SweepScriptData(JSRuntime* rt) {
   // Entries are removed from the table when their reference count is one,
   // i.e. when the only reference to them is from the table entry.
 
   AutoLockScriptData lock(rt);
   ScriptDataTable& table = rt->scriptDataTable(lock);
 
   for (ScriptDataTable::Enum e(table); !e.empty(); e.popFront()) {
     SharedScriptData* scriptData = e.front();
     if (scriptData->refCount() == 1) {
-      scriptData->decRefCount();
+      scriptData->Release();
       e.removeFront();
     }
   }
 }
 
 void js::FreeScriptData(JSRuntime* rt) {
   AutoLockScriptData lock(rt);
 
@@ -3735,19 +3712,17 @@ void JSScript::finalize(FreeOp* fop) {
   }
 #endif
 
   if (data_) {
     AlwaysPoison(data_, 0xdb, computedSizeOfData(), MemCheckKind::MakeNoAccess);
     fop->free_(data_);
   }
 
-  if (scriptData_) {
-    scriptData_->decRefCount();
-  }
+  freeScriptData();
 
   // In most cases, our LazyScript's script pointer will reference this
   // script, and thus be nulled out by normal weakref processing. However, if
   // we unlazified the LazyScript during incremental sweeping, it will have a
   // completely different JSScript.
   MOZ_ASSERT_IF(
       lazyScript && !IsAboutToBeFinalizedUnbarriered(&lazyScript),
       !lazyScript->hasScript() || lazyScript->maybeScriptUnbarriered() != this);
@@ -4238,17 +4213,17 @@ JSScript* js::detail::CopyScript(JSConte
     return nullptr;
   }
 
   // The SharedScriptData can be reused by any zone in the Runtime as long as
   // we make sure to mark first (to sync Atom pointers).
   if (cx->zone() != src->zoneFromAnyThread()) {
     src->scriptData()->markForCrossZone(cx);
   }
-  dst->setScriptData(src->scriptData());
+  dst->scriptData_ = src->scriptData();
 
   return dst;
 }
 
 JSScript* js::CloneGlobalScript(JSContext* cx, ScopeKind scopeKind,
                                 HandleScript src) {
   MOZ_ASSERT(scopeKind == ScopeKind::Global ||
              scopeKind == ScopeKind::NonSyntactic);
--- a/js/src/vm/JSScript.h
+++ b/js/src/vm/JSScript.h
@@ -9,16 +9,17 @@
 #ifndef vm_JSScript_h
 #define vm_JSScript_h
 
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Atomics.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/MaybeOneOf.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/RefPtr.h"
 #include "mozilla/Span.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/Utf8.h"
 #include "mozilla/Variant.h"
 
 #include <type_traits>  // std::is_same
 #include <utility>      // std::move
 
@@ -1473,18 +1474,18 @@ class SharedScriptData {
   uint32_t noteLength_;
   uintptr_t data_[1];
 
  public:
   static SharedScriptData* new_(JSContext* cx, uint32_t codeLength,
                                 uint32_t srcnotesLength, uint32_t natoms);
 
   uint32_t refCount() const { return refCount_; }
-  void incRefCount() { refCount_++; }
-  void decRefCount() {
+  void AddRef() { refCount_++; }
+  void Release() {
     MOZ_ASSERT(refCount_ != 0);
     uint32_t remain = --refCount_;
     if (remain == 0) {
       js_free(this);
     }
   }
 
   size_t dataLength() const {
@@ -1538,22 +1539,21 @@ class SharedScriptData {
   SharedScriptData(const SharedScriptData&) = delete;
   SharedScriptData& operator=(const SharedScriptData&) = delete;
 };
 
 struct ScriptBytecodeHasher {
   class Lookup {
     friend struct ScriptBytecodeHasher;
 
-    SharedScriptData* scriptData;
+    RefPtr<SharedScriptData> scriptData;
     HashNumber hash;
 
    public:
     explicit Lookup(SharedScriptData* data);
-    ~Lookup();
   };
 
   static HashNumber hash(const Lookup& l) { return l.hash; }
   static bool match(SharedScriptData* entry, const Lookup& lookup) {
     const SharedScriptData* data = lookup.scriptData;
     if (entry->natoms() != data->natoms()) {
       return false;
     }
@@ -1593,17 +1593,17 @@ class JSScript : public js::gc::TenuredC
  private:
   // Pointer to baseline->method()->raw(), ion->method()->raw(), a wasm jit
   // entry, the JIT's EnterInterpreter stub, or the lazy link stub. Must be
   // non-null.
   uint8_t* jitCodeRaw_ = nullptr;
   uint8_t* jitCodeSkipArgCheck_ = nullptr;
 
   // Shareable script data
-  js::SharedScriptData* scriptData_ = nullptr;
+  RefPtr<js::SharedScriptData> scriptData_ = {};
 
   // Unshared variable-length data
   js::PrivateScriptData* data_ = nullptr;
 
  public:
   JS::Realm* realm_ = nullptr;
 
  private:
@@ -2532,17 +2532,16 @@ class JSScript : public js::gc::TenuredC
 
  private:
   bool makeTypes(JSContext* cx);
 
   bool createSharedScriptData(JSContext* cx, uint32_t codeLength,
                               uint32_t noteLength, uint32_t natoms);
   bool shareScriptData(JSContext* cx);
   void freeScriptData();
-  void setScriptData(js::SharedScriptData* data);
 
  public:
   uint32_t getWarmUpCount() const { return warmUpCount; }
   uint32_t incWarmUpCounter(uint32_t amount = 1) {
     return warmUpCount += amount;
   }
   uint32_t* addressOfWarmUpCounter() {
     return reinterpret_cast<uint32_t*>(&warmUpCount);