src: fix heap value deduplication in embedder graph

Signed-off-by: ishabi <ilyasshabi94@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/64801
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

src/heap_utils.cc

@@ -57,19 +57,13 @@ class JSGraphJSNode : public EmbedderGraph::Node {
     CHECK(!val.IsEmpty());
   }
 
-  struct Equal {
-    inline bool operator()(JSGraphJSNode* a, JSGraphJSNode* b) const {
-      Local<Data> data_a = a->V8Value();
-      Local<Data> data_b = a->V8Value();
-      if (data_a->IsValue()) {
-        if (!data_b->IsValue()) {
-          return false;
-        }
-        return data_a.As<Value>()->SameValue(data_b.As<Value>());
-      }
-      return data_a == data_b;
+  bool IsSame(Local<Data> other) {
+    Local<Data> value = V8Value();
+    if (value->IsValue() && other->IsValue()) {
+      return value.As<Value>()->SameValue(other.As<Value>());
     }
-  };
+    return value == other;
+  }
 
  private:
   Global<Data> persistent_;
@@ -80,12 +74,15 @@ class JSGraph : public EmbedderGraph {
   explicit JSGraph(Isolate* isolate) : isolate_(isolate) {}
 
   Node* V8Node(const Local<v8::Data>& value) override {
-    std::unique_ptr<JSGraphJSNode> n { new JSGraphJSNode(isolate_, value) };
-    auto it = engine_nodes_.find(n.get());
-    if (it != engine_nodes_.end())
-      return *it;
-    engine_nodes_.insert(n.get());
-    return AddNode(std::unique_ptr<Node>(n.release()));
+    for (JSGraphJSNode* node : engine_nodes_) {
+      if (node->IsSame(value)) {
+        return node;
+      }
+    }
+
+    auto node = std::make_unique<JSGraphJSNode>(isolate_, value);
+    engine_nodes_.push_back(node.get());
+    return AddNode(std::move(node));
   }
 
   Node* V8Node(const Local<v8::Value>& value) override {
@@ -207,7 +204,7 @@ class JSGraph : public EmbedderGraph {
  private:
   Isolate* isolate_;
   std::unordered_set<std::unique_ptr<Node>> nodes_;
-  std::set<JSGraphJSNode*, JSGraphJSNode::Equal> engine_nodes_;
+  std::vector<JSGraphJSNode*> engine_nodes_;
   std::unordered_map<Node*, std::set<std::pair<const char*, Node*>>> edges_;
 };
 
@@ -215,6 +212,11 @@ void BuildEmbedderGraph(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   JSGraph graph(env->isolate());
   Environment::BuildEmbedderGraph(env->isolate(), &graph, env);
+  // This binding is used only by tests. Include supplied values so tests can
+  // verify that JSGraph returns one graph node for each distinct V8 value.
+  for (int i = 0; i < args.Length(); i++) {
+    graph.V8Node(args[i]);
+  }
   Local<Array> ret;
   if (graph.CreateObject().ToLocal(&ret))
     args.GetReturnValue().Set(ret);

test/parallel/test-heap-embedder-graph.js

@@ -0,0 +1,23 @@
+// Flags: --expose-internals
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const { internalBinding } = require('internal/test/binding');
+
+const { buildEmbedderGraph } = internalBinding('heap_utils');
+
+const first = {};
+const second = {};
+const bigint = BigInt('123456789012345678901234567890');
+const sameBigint = BigInt('123456789012345678901234567890');
+const graph = buildEmbedderGraph(first, first, second, bigint, sameBigint);
+
+function findNodes(value) {
+  return graph.filter((node) => Object.hasOwn(node, 'value') &&
+                                Object.is(node.value, value));
+}
+
+assert.strictEqual(findNodes(first).length, 1);
+assert.strictEqual(findNodes(second).length, 1);
+assert.strictEqual(findNodes(bigint).length, 1);