Browse Source

refactor: only override `V8Platform::CreateJob` (#37800)

refactor: only override V8Platform::CreateJob
Shelley Vohr 2 years ago
parent
commit
91f62ae73f

+ 0 - 1
patches/node/.patches

@@ -36,4 +36,3 @@ src_allow_optional_isolation_termination_in_node.patch
 test_mark_cpu_prof_tests_as_flaky_in_electron.patch
 lib_fix_broadcastchannel_initialization_location.patch
 fix_adapt_debugger_tests_for_upstream_v8_changes.patch
-fix_increase_concurrency_in_v8platform_postjob.patch

+ 1 - 1
patches/node/allow_embedder_to_control_codegenerationfromstringscallback.patch

@@ -5,7 +5,7 @@ Subject: allow embedder to control CodeGenerationFromStringsCallback
 
 This is needed to blend Blink and Node's code generation policy.
 
-This should be upstreamed.
+Upstreamed in https://github.com/nodejs/node/pull/46368.
 
 diff --git a/src/api/environment.cc b/src/api/environment.cc
 index a994221445471b92d12ed9cb3bef9ffb70670ab6..d6c6fd9c257cb51ba387c4b4d07a24ff80f9f060 100644

+ 1 - 2
patches/node/api_pass_oomdetails_to_oomerrorcallback.patch

@@ -5,8 +5,7 @@ Subject: Pass OOMDetails to OOMErrorCallback
 
 Introduced in https://chromium-review.googlesource.com/c/v8/v8/+/3647827.
 
-This patch can be removed when Node.js updates to a V8 version containing
-the above CL.
+This patch can be removed when Electron updates to Node.js v20.
 
 diff --git a/src/node_errors.cc b/src/node_errors.cc
 index 9f620154ce3696cff90bf308da934147319b1096..806d9700ca4b5faf46042814c0e2ce212945bece 100644

+ 0 - 32
patches/node/fix_increase_concurrency_in_v8platform_postjob.patch

@@ -1,32 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Shelley Vohr <[email protected]>
-Date: Fri, 31 Mar 2023 17:04:39 +0200
-Subject: fix: increase concurrency in V8Platform::PostJob()
-
-Refs https://chromium-review.googlesource.com/c/v8/v8/+/4347597/11
-
-PostJob posts |job_task| to run in parallel, and so must call NotifyConcurrencyIncrease()
-on the JobTask. In Node.js, the implementations of CreateJob and PostJob were identical,
-meaning that PostJob calls could potentially never run all their tasks.
-
-This was brought to our attention by the above linked V8 CL, which switched a call from
-CreateJob to PostJob and caused a series of failures.
-
-This should be upstreamed.
-
-diff --git a/src/node_platform.cc b/src/node_platform.cc
-index b3994c4398598c67c0029394d58e8f4dba032c5d..f004176d296c8a8ffbd12d44a917d9c7be7b6cf0 100644
---- a/src/node_platform.cc
-+++ b/src/node_platform.cc
-@@ -530,8 +530,10 @@ bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
- 
- std::unique_ptr<v8::JobHandle> NodePlatform::PostJob(v8::TaskPriority priority,
-                                        std::unique_ptr<v8::JobTask> job_task) {
--  return v8::platform::NewDefaultJobHandle(
-+  auto handle = v8::platform::NewDefaultJobHandle(
-       this, priority, std::move(job_task), NumberOfWorkerThreads());
-+  handle->NotifyConcurrencyIncrease();
-+  return handle;
- }
- 
- std::unique_ptr<v8::JobHandle> NodePlatform::CreateJob(v8::TaskPriority priority,

+ 21 - 23
patches/node/fix_override_createjob_in_node_platform.patch

@@ -3,39 +3,37 @@ From: Keeley Hammond <[email protected]>
 Date: Tue, 2 Aug 2022 12:52:02 -0700
 Subject: fix: override createjob in node_platform
 
-This CL changed Platform::CreateJob to an abstract method:
-https://chromium-review.googlesource.com/c/v8/v8/+/3779694
-This patch adds an override for NodePlatform::CreateJob, using
-the same parameters as PostJob.
+Refs https://github.com/nodejs/node/pull/44741/commits/507dd20ef957acf261ec521fcbd81d745b17983c
+
+V8Platform::CreateJob was changed to an abstract method in https://chromium-review.googlesource.com/c/v8/v8/+/3779694,
+and is called by V8Platform::PostJob, so we should only call
+CreateJob in order to most closely match V8Platform default behavior.
+
+This patch can be removed when Electron updates to Node.js v20.
 
 diff --git a/src/node_platform.cc b/src/node_platform.cc
-index 7dd0526e6ece5fd86ab3847be592e778e48b5d37..b3994c4398598c67c0029394d58e8f4dba032c5d 100644
+index 7dd0526e6ece5fd86ab3847be592e778e48b5d37..a20622adb2ee8a2e3b05b336e481193624d5b810 100644
 --- a/src/node_platform.cc
 +++ b/src/node_platform.cc
-@@ -534,6 +534,12 @@ std::unique_ptr<v8::JobHandle> NodePlatform::PostJob(v8::TaskPriority priority,
-       this, priority, std::move(job_task), NumberOfWorkerThreads());
+@@ -528,7 +528,7 @@ bool NodePlatform::FlushForegroundTasks(Isolate* isolate) {
+   return per_isolate->FlushForegroundTasksInternal();
  }
  
+-std::unique_ptr<v8::JobHandle> NodePlatform::PostJob(v8::TaskPriority priority,
 +std::unique_ptr<v8::JobHandle> NodePlatform::CreateJob(v8::TaskPriority priority,
-+                                       std::unique_ptr<v8::JobTask> job_task) {
-+  return v8::platform::NewDefaultJobHandle(
-+      this, priority, std::move(job_task), NumberOfWorkerThreads());
-+}
-+
- bool NodePlatform::IdleTasksEnabled(Isolate* isolate) {
-   return ForIsolate(isolate)->IdleTasksEnabled();
- }
+                                        std::unique_ptr<v8::JobTask> job_task) {
+   return v8::platform::NewDefaultJobHandle(
+       this, priority, std::move(job_task), NumberOfWorkerThreads());
 diff --git a/src/node_platform.h b/src/node_platform.h
-index 4a05f3bba58c8e875d0ab67f292589edbb3b812b..b8a956c286a5ea88b8b520322e04b4e4e16a2591 100644
+index 4a05f3bba58c8e875d0ab67f292589edbb3b812b..1062f3b1b9c386a7bde8dca366c6f008bb183ab7 100644
 --- a/src/node_platform.h
 +++ b/src/node_platform.h
-@@ -158,6 +158,9 @@ class NodePlatform : public MultiIsolatePlatform {
-   std::unique_ptr<v8::JobHandle> PostJob(
+@@ -155,7 +155,7 @@ class NodePlatform : public MultiIsolatePlatform {
+   double CurrentClockTimeMillis() override;
+   v8::TracingController* GetTracingController() override;
+   bool FlushForegroundTasks(v8::Isolate* isolate) override;
+-  std::unique_ptr<v8::JobHandle> PostJob(
++  std::unique_ptr<v8::JobHandle> CreateJob(
        v8::TaskPriority priority,
        std::unique_ptr<v8::JobTask> job_task) override;
-+  std::unique_ptr<v8::JobHandle> CreateJob(
-+      v8::TaskPriority priority,
-+      std::unique_ptr<v8::JobTask> job_task) override;
  
-   void RegisterIsolate(v8::Isolate* isolate, uv_loop_t* loop) override;
-   void RegisterIsolate(v8::Isolate* isolate,

+ 1 - 1
patches/node/lib_fix_broadcastchannel_initialization_location.patch

@@ -10,7 +10,7 @@ initialized in bootstrap/browser instead of bootstrap/node. That
 inadvertently made it such that there was incorrect handling of the
 DOM vs Node.js implementations of BroadcastChannel.
 
-This will be upstreamed.
+Upstreamed in https://github.com/nodejs/node/pull/46864.
 
 diff --git a/lib/internal/bootstrap/browser.js b/lib/internal/bootstrap/browser.js
 index d0c01ca2a512be549b0fea8a829c05eabbec799a..210a1bb7e929021725b04786bc11d9b3ce09ad04 100644

+ 2 - 1
patches/node/v8_api_advance_api_deprecation.patch

@@ -4,7 +4,8 @@ Date: Fri, 26 Aug 2022 00:03:44 +0900
 Subject: v8: [api] Advance API deprecation
 
 Refs https://chromium-review.googlesource.com/c/v8/v8/+/3702449
-Should be upstreamed.
+
+This can be removed when Electron upgrades to Node.js v20.
 
 diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
 index 1abf6801e0544b14dd26f0b96536bd78c5b01679..93e339602800a21726c9414bf2ebe9a2e5517a3b 100644