Bug 1689102 - Part 2: Refactor AbstractScopePtr not to hold Scope pointer. r=mgaudet

Instead of accessing Scope from AbstractScopePtr, pre-calculate necessary
information in ScopeContext.

Also removed GCThingList.stencil field that's now unnecessary.

Depends on D103453

Differential Revision: https://phabricator.services.mozilla.com/D103454

js/src/frontend/AbstractScopePtr.cpp

@@ -17,50 +17,45 @@ using namespace js;
 using namespace js::frontend;
 
 ScopeStencil& AbstractScopePtr::scopeData() const {
-  const Deferred& data = scope_.as<Deferred>();
-  return data.compilationState.scopeData[data.index];
-}
-
-CompilationState& AbstractScopePtr::compilationState() const {
-  const Deferred& data = scope_.as<Deferred>();
-  return data.compilationState;
+  MOZ_ASSERT(isScopeStencil());
+  return compilationState_.scopeData[index_];
 }
 
 ScopeKind AbstractScopePtr::kind() const {
-  MOZ_ASSERT(!isNullptr());
   if (isScopeStencil()) {
     return scopeData().kind();
   }
-  return scope()->kind();
+  return compilationState_.scopeContext.enclosingScopeKind;
 }
 
 AbstractScopePtr AbstractScopePtr::enclosing() const {
-  MOZ_ASSERT(!isNullptr());
-  if (isScopeStencil()) {
-    return scopeData().enclosing(compilationState());
-  }
-  return AbstractScopePtr(scope()->enclosing());
+  MOZ_ASSERT(isScopeStencil());
+  return scopeData().enclosing(compilationState_);
 }
 
 bool AbstractScopePtr::hasEnvironment() const {
-  MOZ_ASSERT(!isNullptr());
   if (isScopeStencil()) {
     return scopeData().hasEnvironment();
   }
-  return scope()->hasEnvironment();
+  return compilationState_.scopeContext.enclosingScopeHasEnvironment;
 }
 
 bool AbstractScopePtr::isArrow() const {
-  // nullptr will also fail the below assert, so effectively also checking
-  // !isNullptr()
   MOZ_ASSERT(is<FunctionScope>());
   if (isScopeStencil()) {
     return scopeData().isArrow();
   }
-  MOZ_ASSERT(scope()->as<FunctionScope>().canonicalFunction());
-  return scope()->as<FunctionScope>().canonicalFunction()->isArrow();
+  return compilationState_.scopeContext.enclosingScopeIsArrow;
 }
 
-void AbstractScopePtr::trace(JSTracer* trc) {
-  JS::GCPolicy<ScopeType>::trace(trc, &scope_, "AbstractScopePtr");
+#ifdef DEBUG
+bool AbstractScopePtr::hasNonSyntacticScopeOnChain() const {
+  if (isScopeStencil()) {
+    if (kind() == ScopeKind::NonSyntactic) {
+      return true;
+    }
+    return enclosing().hasNonSyntacticScopeOnChain();
+  }
+  return compilationState_.scopeContext.hasNonSyntacticScopeOnChain;
 }
+#endif

js/src/frontend/AbstractScopePtr.h

@@ -30,8 +30,17 @@ struct CompilationGCOutput;
 class ScopeStencil;
 }  // namespace frontend
 
-using ScopeIndex = frontend::TypedIndex<Scope>;
-using HeapPtrScope = HeapPtr<Scope*>;
+class ScopeIndex : public frontend::TypedIndex<Scope> {
+  // Delegate constructors;
+  using Base = frontend::TypedIndex<Scope>;
+  using Base::Base;
+
+  static constexpr uint32_t InvalidIndex = UINT32_MAX;
+
+ public:
+  static constexpr ScopeIndex invalid() { return ScopeIndex(InvalidIndex); }
+  bool isValid() const { return index != InvalidIndex; }
+};
 
 // An interface class to support Scope queries in the frontend without requiring
 // a GC Allocated scope to necessarily exist.
@@ -39,65 +48,38 @@ using HeapPtrScope = HeapPtr<Scope*>;
 // This abstracts Scope* and a ScopeStencil type used within the frontend before
 // the Scope is allocated.
 //
-// Because a AbstractScopePtr may hold onto a Scope, it must be rooted if a GC
-// may occur to ensure that the scope is traced.
+// Queries to GC Scope should be pre-calculated and stored into ScopeContext.
 class AbstractScopePtr {
- public:
-  // Used to hold index and the compilationState together to avoid having a
-  // potentially nullable compilationState.
-  struct Deferred {
-    ScopeIndex index;
-    frontend::CompilationState& compilationState;
-  };
-
-  // To make writing code and managing invariants easier, we require that
-  // any nullptr scopes be stored on the HeapPtrScope arm of the variant.
-  using ScopeType = mozilla::Variant<HeapPtrScope, Deferred>;
-
  private:
-  ScopeType scope_ = ScopeType(HeapPtrScope());
+  // ScopeIndex::invalid() if this points CompilationInput.enclosingScope.
+  ScopeIndex index_;
 
-  Scope* scope() const { return scope_.as<HeapPtrScope>(); }
+  frontend::CompilationState& compilationState_;
 
  public:
   friend class js::Scope;
 
-  AbstractScopePtr() = default;
-
-  explicit AbstractScopePtr(Scope* scope) : scope_(HeapPtrScope(scope)) {}
-
   AbstractScopePtr(frontend::CompilationState& compilationState,
-                   ScopeIndex scope)
-      : scope_(Deferred{scope, compilationState}) {}
+                   ScopeIndex index)
+      : index_(index), compilationState_(compilationState) {}
 
-  bool isNullptr() const {
-    if (isScopeStencil()) {
-      return false;
-    }
-    return scope_.as<HeapPtrScope>() == nullptr;
+  static AbstractScopePtr compilationEnclosingScope(
+      frontend::CompilationState& compilationState) {
+    return AbstractScopePtr(compilationState, ScopeIndex::invalid());
   }
 
-  // Return true if this AbstractScopePtr represents a Scope, either existant
-  // or to be reified. This indicates that queries can be executed on this
-  // scope data. Returning false is the equivalent to a nullptr, and usually
-  // indicates the end of the scope chain.
-  explicit operator bool() const { return !isNullptr(); }
+ private:
+  bool isScopeStencil() const { return index_.isValid(); }
 
-  bool isScopeStencil() const { return scope_.is<Deferred>(); }
-
-  // Note: this handle is rooted in the CompilationState.
   frontend::ScopeStencil& scopeData() const;
-  frontend::CompilationState& compilationState() const;
 
+ public:
   // This allows us to check whether or not this provider wraps
   // or otherwise would reify to a particular scope type.
   template <typename T>
   bool is() const {
     static_assert(std::is_base_of_v<Scope, T>,
                   "Trying to ask about non-Scope type");
-    if (isNullptr()) {
-      return false;
-    }
     return kind() == T::classScopeKind_;
   }
 
@@ -107,37 +89,22 @@ class AbstractScopePtr {
   // Valid iff is<FunctionScope>
   bool isArrow() const;
 
-  bool hasOnChain(ScopeKind kind) const {
-    for (AbstractScopePtr it = *this; it; it = it.enclosing()) {
-      if (it.kind() == kind) {
-        return true;
-      }
-    }
-    return false;
-  }
-
-  void trace(JSTracer* trc);
+#ifdef DEBUG
+  bool hasNonSyntacticScopeOnChain() const;
+#endif
 };
 
 // Specializations of AbstractScopePtr::is
 template <>
 inline bool AbstractScopePtr::is<GlobalScope>() const {
-  return !isNullptr() &&
-         (kind() == ScopeKind::Global || kind() == ScopeKind::NonSyntactic);
+  return kind() == ScopeKind::Global || kind() == ScopeKind::NonSyntactic;
 }
 
 template <>
 inline bool AbstractScopePtr::is<EvalScope>() const {
-  return !isNullptr() &&
-         (kind() == ScopeKind::Eval || kind() == ScopeKind::StrictEval);
+  return kind() == ScopeKind::Eval || kind() == ScopeKind::StrictEval;
 }
 
 }  // namespace js
 
-namespace JS {
-template <>
-struct GCPolicy<js::AbstractScopePtr::Deferred>
-    : JS::IgnoreGCPolicy<js::AbstractScopePtr::Deferred> {};
-}  // namespace JS
-
 #endif  // frontend_AbstractScopePtr_h

js/src/frontend/BytecodeEmitter.cpp

@@ -124,7 +124,7 @@ BytecodeEmitter::BytecodeEmitter(BytecodeEmitter* parent, SharedContext* sc,
       cx(sc->cx_),
       parent(parent),
       bytecodeSection_(cx, sc->extent().lineno, sc->extent().column),
-      perScriptData_(cx, stencil, compilationState),
+      perScriptData_(cx, compilationState),
       stencil(stencil),
       compilationState(compilationState),
       emitterMode(emitterMode) {}
@@ -11239,7 +11239,7 @@ bool BytecodeEmitter::intoScriptStencil(ScriptIndex scriptIndex) {
     return false;
   }
 
-  MOZ_ASSERT(outermostScope().hasOnChain(ScopeKind::NonSyntactic) ==
+  MOZ_ASSERT(outermostScope().hasNonSyntacticScopeOnChain() ==
              sc->hasNonSyntacticScope());
 
   auto& things = perScriptData().gcThingList().objects();

js/src/frontend/BytecodeSection.cpp

@@ -36,9 +36,7 @@ AbstractScopePtr GCThingList::getScope(size_t index) const {
   if (elem.isEmptyGlobalScope()) {
     // The empty enclosing scope should be stored by
     // CompilationInput::initForSelfHostingGlobal.
-    MOZ_ASSERT(stencil.input.enclosingScope);
-    MOZ_ASSERT(!stencil.input.enclosingScope->as<GlobalScope>().hasBindings());
-    return AbstractScopePtr(stencil.input.enclosingScope);
+    return AbstractScopePtr::compilationEnclosingScope(compilationState);
   }
   return AbstractScopePtr(compilationState, elem.toScope());
 }
@@ -190,9 +188,8 @@ void BytecodeSection::updateDepth(BytecodeOffset target) {
 }
 
 PerScriptData::PerScriptData(JSContext* cx,
-                             frontend::CompilationStencil& stencil,
                              frontend::CompilationState& compilationState)
-    : gcThingList_(cx, stencil, compilationState),
+    : gcThingList_(cx, compilationState),
       atomIndices_(cx->frontendCollectionPool()) {}
 
 bool PerScriptData::init(JSContext* cx) { return atomIndices_.acquire(cx); }

js/src/frontend/BytecodeSection.h

@@ -49,16 +49,14 @@ struct MOZ_STACK_CLASS GCThingList {
   // reserve some stack slots to avoid allocating for most small scripts.
   using ScriptThingsStackVector = Vector<TaggedScriptThingIndex, 8>;
 
-  CompilationStencil& stencil;
   CompilationState& compilationState;
   ScriptThingsStackVector vector;
 
   // Index of the first scope in the vector.
   mozilla::Maybe<GCThingIndex> firstScopeIndex;
 
-  explicit GCThingList(JSContext* cx, CompilationStencil& stencil,
-                       CompilationState& compilationState)
-      : stencil(stencil), compilationState(compilationState), vector(cx) {}
+  explicit GCThingList(JSContext* cx, CompilationState& compilationState)
+      : compilationState(compilationState), vector(cx) {}
 
   MOZ_MUST_USE bool append(const ParserAtom* atom, GCThingIndex* index) {
     *index = GCThingIndex(vector.length());
@@ -393,7 +391,7 @@ class BytecodeSection {
 // bytecode, but referred from bytecode is stored in this class.
 class PerScriptData {
  public:
-  explicit PerScriptData(JSContext* cx, frontend::CompilationStencil& stencil,
+  explicit PerScriptData(JSContext* cx,
                          frontend::CompilationState& compilationState);
 
   MOZ_MUST_USE bool init(JSContext* cx);

js/src/frontend/CompilationInfo.h

@@ -32,6 +32,7 @@
 #include "vm/JSFunction.h"  // JSFunction
 #include "vm/JSScript.h"    // SourceExtent
 #include "vm/Realm.h"
+#include "vm/ScopeKind.h"      // ScopeKind
 #include "vm/SharedStencil.h"  // SharedImmutableScriptData
 
 namespace js {
@@ -65,6 +66,11 @@ struct ScopeContext {
   // compiling is not an eval or arrow-function.
   uint32_t enclosingThisEnvironmentHops = 0;
 
+  // If non-null enclosingScope is passed to constructor, the kind of the scope.
+  // If null enclosingScope is passed instead, the compilation should use
+  // empty global scope.
+  ScopeKind enclosingScopeKind = ScopeKind::Global;
+
   // The type of binding required for `this` of the top level context, as
   // indicated by the enclosing scopes of this parse.
   //
@@ -82,20 +88,32 @@ struct ScopeContext {
   bool inClass = false;
   bool inWith = false;
 
-  explicit ScopeContext(JSContext* cx, InheritThis inheritThis, Scope* scope,
-                        JSObject* enclosingEnv = nullptr)
-      : effectiveScope(cx, determineEffectiveScope(scope, enclosingEnv)) {
+  // True if the passed enclosingScope is for FunctionScope of arrow function.
+  bool enclosingScopeIsArrow = false;
+
+  // True if the passed enclosingScope has environment.
+  bool enclosingScopeHasEnvironment = false;
+
+#ifdef DEBUG
+  // True if the passed enclosingScope has non-syntactic scope on chain.
+  bool hasNonSyntacticScopeOnChain = false;
+#endif
+
+  explicit ScopeContext(JSContext* cx, InheritThis inheritThis,
+                        Scope* enclosingScope, JSObject* enclosingEnv = nullptr)
+      : effectiveScope(cx,
+                       determineEffectiveScope(enclosingScope, enclosingEnv)) {
     if (inheritThis == InheritThis::Yes) {
       computeThisBinding(effectiveScope);
-      computeThisEnvironment(scope);
+      computeThisEnvironment(enclosingScope);
     }
-    computeInScope(scope);
+    computeInScope(enclosingScope);
   }
 
  private:
   void computeThisBinding(Scope* scope);
-  void computeThisEnvironment(Scope* scope);
-  void computeInScope(Scope* scope);
+  void computeThisEnvironment(Scope* enclosingScope);
+  void computeInScope(Scope* enclosingScope);
 
   static Scope* determineEffectiveScope(Scope* scope, JSObject* environment);
 };

js/src/frontend/SharedContext.cpp

@@ -70,9 +70,9 @@ SharedContext::SharedContext(JSContext* cx, Kind kind,
   setFlag(ImmutableFlags::Strict, directives.strict());
 }
 
-void ScopeContext::computeThisEnvironment(Scope* scope) {
+void ScopeContext::computeThisEnvironment(Scope* enclosingScope) {
   uint32_t envCount = 0;
-  for (ScopeIter si(scope); si; si++) {
+  for (ScopeIter si(enclosingScope); si; si++) {
     if (si.kind() == ScopeKind::Function) {
       JSFunction* fun = si.scope()->as<FunctionScope>().canonicalFunction();
 
@@ -145,8 +145,23 @@ void ScopeContext::computeThisBinding(Scope* scope) {
   thisBinding = ThisBinding::Global;
 }
 
-void ScopeContext::computeInScope(Scope* scope) {
-  for (ScopeIter si(scope); si; si++) {
+void ScopeContext::computeInScope(Scope* enclosingScope) {
+  if (enclosingScope) {
+    enclosingScopeKind = enclosingScope->kind();
+    if (enclosingScope->is<FunctionScope>()) {
+      MOZ_ASSERT(enclosingScope->as<FunctionScope>().canonicalFunction());
+      enclosingScopeIsArrow =
+          enclosingScope->as<FunctionScope>().canonicalFunction()->isArrow();
+    }
+    enclosingScopeHasEnvironment = enclosingScope->hasEnvironment();
+
+#ifdef DEBUG
+    hasNonSyntacticScopeOnChain =
+        enclosingScope->hasOnChain(ScopeKind::NonSyntactic);
+#endif
+  }
+
+  for (ScopeIter si(enclosingScope); si; si++) {
     if (si.kind() == ScopeKind::ClassBody) {
       inClass = true;
     }

js/src/frontend/Stencil.cpp

@@ -50,7 +50,7 @@ AbstractScopePtr ScopeStencil::enclosing(
     return AbstractScopePtr(compilationState, enclosing());
   }
 
-  return AbstractScopePtr(compilationState.input.enclosingScope);
+  return AbstractScopePtr::compilationEnclosingScope(compilationState);
 }
 
 Scope* ScopeStencil::enclosingExistingScope(

js/src/vm/Scope.cpp

@@ -1982,7 +1982,7 @@ template <typename... Args>
 /* static */ bool ScopeStencil::appendScopeStencilAndData(
     JSContext* cx, CompilationState& compilationState,
     BaseParserScopeData* data, ScopeIndex* indexOut, Args&&... args) {
-  *indexOut = compilationState.scopeData.length();
+  *indexOut = ScopeIndex(compilationState.scopeData.length());
   if (uint32_t(*indexOut) >= TaggedScriptThingIndex::IndexLimit) {
     ReportAllocationOverflow(cx);
     return false;