vm: migrate ContextifyScript to cppgc

This patch migrates ContextifyScript to cppgc-based memory management using CppgcMixin. PR-URL: nodejs#52295 Refs: nodejs#40786 Refs: https://docs.google.com/document/d/1ny2Qz_EsUnXGKJRGxoA-FXIE2xpLgaMAN6jD7eAkqFQ/edit Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
XAuthSystems · Aug 30, 2024 · 3b0617d · 3b0617d
1 parent 849db10
commit 3b0617d
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 54 deletions.
diff --git a/benchmark/vm/compile-script-in-isolate-cache.js b/benchmark/vm/compile-script-in-isolate-cache.js
@@ -5,17 +5,17 @@
 const common = require('../common.js');
 const fs = require('fs');
 const vm = require('vm');
-const fixtures = require('../../test/common/fixtures.js');
-const scriptPath = fixtures.path('snapshot', 'typescript.js');
+const path = require('path');
 
 const bench = common.createBenchmark(main, {
   type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'],
-  n: [100],
+  filename: ['test/fixtures/snapshot/typescript.js', 'test/fixtures/syntax/good_syntax.js'],
+  n: [1000],
 });
 
-const scriptSource = fs.readFileSync(scriptPath, 'utf8');
-
-function main({ n, type }) {
+function main({ n, type, filename }) {
+  const scriptPath = path.resolve(__dirname, '..', '..', filename);
+  const scriptSource = fs.readFileSync(scriptPath, 'utf8');
   let script;
   bench.start();
   const options = {};

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -22,6 +22,7 @@
 #include "node_contextify.h"
 
 #include "base_object-inl.h"
+#include "cppgc/allocation.h"
 #include "memory_tracker-inl.h"
 #include "module_wrap.h"
 #include "node_context_data.h"
@@ -960,6 +961,12 @@ void ContextifyScript::RegisterExternalReferences(
   registry->Register(RunInContext);
 }
 
+ContextifyScript* ContextifyScript::New(Environment* env,
+                                        Local<Object> object) {
+  return cppgc::MakeGarbageCollected<ContextifyScript>(
+      env->isolate()->GetCppHeap()->GetAllocationHandle(), env, object);
+}
+
 void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = env->isolate();
@@ -1010,8 +1017,7 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
     id_symbol = args[7].As<Symbol>();
   }
 
-  ContextifyScript* contextify_script =
-      new ContextifyScript(env, args.This());
+  ContextifyScript* contextify_script = New(env, args.This());
 
   if (*TRACE_EVENT_API_GET_CATEGORY_GROUP_ENABLED(
           TRACING_CATEGORY_NODE2(vm, script)) != 0) {
@@ -1069,9 +1075,7 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
-  contextify_script->script_.Reset(isolate, v8_script);
-  contextify_script->script_.SetWeak();
-  contextify_script->object()->SetInternalField(kUnboundScriptSlot, v8_script);
+  contextify_script->set_unbound_script(v8_script);
 
   std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
   if (produce_cached_data) {
@@ -1174,11 +1178,9 @@ void ContextifyScript::CreateCachedData(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   ContextifyScript* wrapped_script;
-  ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.This());
-  Local<UnboundScript> unbound_script =
-      PersistentToLocal::Default(env->isolate(), wrapped_script->script_);
+  ASSIGN_OR_RETURN_UNWRAP_CPPGC(&wrapped_script, args.This());
   std::unique_ptr<ScriptCompiler::CachedData> cached_data(
-      ScriptCompiler::CreateCodeCache(unbound_script));
+      ScriptCompiler::CreateCodeCache(wrapped_script->unbound_script()));
   if (!cached_data) {
     args.GetReturnValue().Set(Buffer::New(env, 0).ToLocalChecked());
   } else {
@@ -1192,9 +1194,8 @@ void ContextifyScript::CreateCachedData(
 
 void ContextifyScript::RunInContext(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-
   ContextifyScript* wrapped_script;
-  ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.This());
+  ASSIGN_OR_RETURN_UNWRAP_CPPGC(&wrapped_script, args.This());
 
   CHECK_EQ(args.Length(), 5);
   CHECK(args[0]->IsObject() || args[0]->IsNull());
@@ -1264,10 +1265,9 @@ bool ContextifyScript::EvalMachine(Local<Context> context,
 
   TryCatchScope try_catch(env);
   ContextifyScript* wrapped_script;
-  ASSIGN_OR_RETURN_UNWRAP(&wrapped_script, args.This(), false);
-  Local<UnboundScript> unbound_script =
-      PersistentToLocal::Default(env->isolate(), wrapped_script->script_);
-  Local<Script> script = unbound_script->BindToCurrentContext();
+  ASSIGN_OR_RETURN_UNWRAP_CPPGC(&wrapped_script, args.This(), false);
+  Local<Script> script =
+      wrapped_script->unbound_script()->BindToCurrentContext();
 
 #if HAVE_INSPECTOR
   if (break_on_first_line) {
@@ -1349,9 +1349,21 @@ bool ContextifyScript::EvalMachine(Local<Context> context,
   return true;
 }
 
-ContextifyScript::ContextifyScript(Environment* env, Local<Object> object)
-    : BaseObject(env, object) {
-  MakeWeak();
+Local<UnboundScript> ContextifyScript::unbound_script() const {
+  return script_.Get(env()->isolate());
+}
+
+void ContextifyScript::set_unbound_script(Local<UnboundScript> script) {
+  script_.Reset(env()->isolate(), script);
+}
+
+void ContextifyScript::Trace(cppgc::Visitor* visitor) const {
+  CppgcMixin::Trace(visitor);
+  visitor->Trace(script_);
+}
+
+ContextifyScript::ContextifyScript(Environment* env, Local<Object> object) {
+  CppgcMixin::Wrap(this, env, object);
 }
 
 ContextifyScript::~ContextifyScript() {}

diff --git a/src/node_contextify.h b/src/node_contextify.h
@@ -4,6 +4,7 @@
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
 #include "base_object-inl.h"
+#include "cppgc_helpers.h"
 #include "node_context_data.h"
 #include "node_errors.h"
 
@@ -143,23 +144,21 @@ class ContextifyContext : public BaseObject {
   std::unique_ptr<v8::MicrotaskQueue> microtask_queue_;
 };
 
-class ContextifyScript : public BaseObject {
+class ContextifyScript final : CPPGC_MIXIN(ContextifyScript) {
  public:
-  enum InternalFields {
-    kUnboundScriptSlot = BaseObject::kInternalFieldCount,
-    kInternalFieldCount
-  };
-
-  SET_NO_MEMORY_INFO()
-  SET_MEMORY_INFO_NAME(ContextifyScript)
-  SET_SELF_SIZE(ContextifyScript)
+  SET_CPPGC_NAME(ContextifyScript)
+  void Trace(cppgc::Visitor* visitor) const final;
 
   ContextifyScript(Environment* env, v8::Local<v8::Object> object);
   ~ContextifyScript() override;
 
+  v8::Local<v8::UnboundScript> unbound_script() const;
+  void set_unbound_script(v8::Local<v8::UnboundScript>);
+
   static void CreatePerIsolateProperties(IsolateData* isolate_data,
                                          v8::Local<v8::ObjectTemplate> target);
   static void RegisterExternalReferences(ExternalReferenceRegistry* registry);
+  static ContextifyScript* New(Environment* env, v8::Local<v8::Object> object);
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static bool InstanceOf(Environment* env, const v8::Local<v8::Value>& args);
   static void CreateCachedData(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -174,7 +173,7 @@ class ContextifyScript : public BaseObject {
                           const v8::FunctionCallbackInfo<v8::Value>& args);
 
  private:
-  v8::Global<v8::UnboundScript> script_;
+  v8::TracedReference<v8::UnboundScript> script_;
 };
 
 v8::Maybe<bool> StoreCodeCacheResult(

diff --git a/test/pummel/test-heapdump-env.js b/test/pummel/test-heapdump-env.js
@@ -2,17 +2,12 @@
 'use strict';
 
 // This tests that Environment is tracked in heap snapshots.
+// Tests for BaseObject and cppgc-managed objects are done in other
+// test-heapdump-*.js files.
 
 require('../common');
 const { validateSnapshotNodes } = require('../common/heap');
 
-// This is just using ContextifyScript as an example here, it can be replaced
-// with any BaseObject that we can easily instantiate here and register in
-// cleanup hooks.
-// These can all be changed to reflect the status of how these objects
-// are captured in the snapshot.
-const context = require('vm').createScript('const foo = 123');
-
 validateSnapshotNodes('Node / Environment', [{
   children: [
     { node_name: 'Node / CleanupQueue', edge_name: 'cleanup_queue' },
@@ -21,21 +16,8 @@ validateSnapshotNodes('Node / Environment', [{
   ],
 }]);
 
-validateSnapshotNodes('Node / CleanupQueue', [
-  // The first one is the cleanup_queue of the Environment.
-  {},
-  // The second one is the cleanup_queue of the principal realm.
-  {
-    children: [
-      { node_name: 'Node / ContextifyScript' },
-    ],
-  },
-]);
-
 validateSnapshotNodes('Node / PrincipalRealm', [{
   children: [
     { node_name: 'process', edge_name: 'process_object' },
   ],
 }]);
-
-console.log(context);  // Make sure it's not GC'ed
diff --git a/test/pummel/test-heapdump-vm-script.js b/test/pummel/test-heapdump-vm-script.js
@@ -0,0 +1,13 @@
+'use strict';
+require('../common');
+const { findByRetainingPath } = require('../common/heap');
+const source = 'const foo = 123';
+const script = require('vm').createScript(source);
+
+findByRetainingPath('Node / ContextifyScript', [
+  { node_name: '(shared function info)' },  // This is the UnboundScript referenced by ContextifyScript.
+  { edge_name: 'script' },
+  { edge_name: 'source', node_type: 'string', node_name: source },
+]);
+
+console.log(script); // Keep the script alive.