src: always compile and store code cache for native modules #24950

joyeecheung · 2018-12-10T23:54:15Z

src: remove code cache integrity check

In preparation of sharing code cache among different threads -
we simply rely on v8 to reject invalid cache, since there isn't
any serious consequence when the cache is invalid anyway.

src: always compile and store code cache for native modules

This patch changes the NativeModuleLoader to always try to find
code cache for native modules when it compiles them, and always
produce and store the code cache after compilation. The cache
map is protected by a mutex and can be accessed by different
threads - including the worker threads and the main thread. Hence any
thread can reuse the code cache if the native module has already
been compiled by another thread - in particular the cache of the
bootstrappers and per_context.js will always be hit when a new thread
is spun.

This results in a ~6% startup overhead in the worst case
(when only the main thread is launched without requiring any additional
native module - it now needs to do the extra work of finding and
storing caches), which balances out the recent improvements by moving
the compilation to C++, but it also leads to a ~60% improvement in
the best case (when a worker thread is spun and requires a lot of native
modules thus hitting the cache compiled by the main thread).

Local benchmark results:

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***     -6.80 %       ±0.77% ±1.02% ±1.34%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***     -4.90 %       ±0.82% ±1.10% ±1.43%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     63.13 %       ±1.58% ±2.12% ±2.79%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***     56.20 %       ±1.33% ±1.78% ±2.33%

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway.

This patch changes the NativeModuleLoader to always try to find code cache for native modules when it compiles them, and always produce and store the code cache after compilation. The cache map is protected by a mutex and can be accessed by different threads - including the worker threads and the main thread. Hence any thread can reuse the code cache if the native module has already been compiled by another thread - in particular the cache of the bootstrappers and per_context.js will always be hit when a new thread is spun. This results in a ~6% startup overhead in the worst case (when only the main thread is launched without requiring any additional native module - it now needs to do the extra work of finding and storing caches), which balances out the recent improvements by moving the compilation to C++, but it also leads to a ~60% improvement in the best case (when a worker thread is spun and requires a lot of native modules thus hitting the cache compiled by the main thread).

nodejs-github-bot · 2018-12-10T23:54:17Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1890/pipeline

joyeecheung · 2018-12-10T23:55:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/19408/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/281/

src/node_native_module.cc

…ules

joyeecheung · 2018-12-11T00:47:51Z

@addaleax Thanks for the review, updated. Also updated the emplace() calls since gcc 4.9.4 needs the pointers to be moved before the call, and updated the test to pass the --worker test suit since compileWithCache is no longer guaranteed to be empty when a worker thread is spun (that's the whole point of this patch :p)

PTAL.

Also cc @nodejs/process @nodejs/build-files

CI: https://ci.nodejs.org/job/node-test-pull-request/19412/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/282/

joyeecheung · 2018-12-12T16:50:13Z

Removing author-ready because this needs at least one more approval to land before next week.

From the previous benchmark CI:

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***     -7.38 %       ±1.09% ±1.46% ±1.90%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***     -5.12 %       ±0.79% ±1.06% ±1.37%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     54.77 %       ±1.42% ±1.91% ±2.50%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***     48.22 %       ±3.27% ±4.40% ±5.82%

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/19454/

BridgeAR · 2018-12-12T16:56:31Z

@joyeecheung the label is not meant as ready to land but the author is done. Missing LGs are absolutely fine while having the author ready label applied :-)

joyeecheung · 2018-12-12T17:21:06Z

@BridgeAR

the label is not meant as ready to land but the author is done. Missing LGs are absolutely fine while having the author ready label applied :-)

That seems to contradict the label description:

PRs that have at least two approvals, no pending requests for changes, and a CI started.

And the collaborator guide:

at least two Collaborators approved the PR (one Collaborator approval is enough if the pull request has been open for more than 7 days).

BridgeAR · 2018-12-13T14:30:37Z

@joyeecheung

That seems to contradict the label description:

The label was not up to date. It's fixed now.

And the collaborator guide:

Thanks! Seems like I missed that in #24893. I am opening a PR to fix that right away.

joyeecheung · 2018-12-18T05:12:27Z

CI before landing: https://ci.nodejs.org/job/node-test-pull-request/19636/

joyeecheung · 2018-12-18T10:03:27Z

Landed in ceb6635 and 0858e5d

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway. PR-URL: #24950 Reviewed-By: Anna Henningsen <[email protected]>

This patch changes the NativeModuleLoader to always try to find code cache for native modules when it compiles them, and always produce and store the code cache after compilation. The cache map is protected by a mutex and can be accessed by different threads - including the worker threads and the main thread. Hence any thread can reuse the code cache if the native module has already been compiled by another thread - in particular the cache of the bootstrappers and per_context.js will always be hit when a new thread is spun. This results in a ~6% startup overhead in the worst case (when only the main thread is launched without requiring any additional native module - it now needs to do the extra work of finding and storing caches), which balances out the recent improvements by moving the compilation to C++, but it also leads to a ~60% improvement in the best case (when a worker thread is spun and requires a lot of native modules thus hitting the cache compiled by the main thread). PR-URL: #24950 Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-12-25T06:34:58Z

This does not land cleanly on v11.x, would someone be willing to backport?

BridgeAR · 2019-01-09T01:01:59Z

Ping @joyeecheung

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway. PR-URL: #24950 Reviewed-By: Anna Henningsen <[email protected]>

This patch changes the NativeModuleLoader to always try to find code cache for native modules when it compiles them, and always produce and store the code cache after compilation. The cache map is protected by a mutex and can be accessed by different threads - including the worker threads and the main thread. Hence any thread can reuse the code cache if the native module has already been compiled by another thread - in particular the cache of the bootstrappers and per_context.js will always be hit when a new thread is spun. This results in a ~6% startup overhead in the worst case (when only the main thread is launched without requiring any additional native module - it now needs to do the extra work of finding and storing caches), which balances out the recent improvements by moving the compilation to C++, but it also leads to a ~60% improvement in the best case (when a worker thread is spun and requires a lot of native modules thus hitting the cache compiled by the main thread). PR-URL: #24950 Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2019-01-14T14:42:04Z

Lands cleanly now 👍

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway. PR-URL: nodejs#24950 Reviewed-By: Anna Henningsen <[email protected]>

This patch changes the NativeModuleLoader to always try to find code cache for native modules when it compiles them, and always produce and store the code cache after compilation. The cache map is protected by a mutex and can be accessed by different threads - including the worker threads and the main thread. Hence any thread can reuse the code cache if the native module has already been compiled by another thread - in particular the cache of the bootstrappers and per_context.js will always be hit when a new thread is spun. This results in a ~6% startup overhead in the worst case (when only the main thread is launched without requiring any additional native module - it now needs to do the extra work of finding and storing caches), which balances out the recent improvements by moving the compilation to C++, but it also leads to a ~60% improvement in the best case (when a worker thread is spun and requires a lot of native modules thus hitting the cache compiled by the main thread). PR-URL: nodejs#24950 Reviewed-By: Anna Henningsen <[email protected]>

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway. PR-URL: nodejs#24950 Reviewed-By: Anna Henningsen <[email protected]>

This patch changes the NativeModuleLoader to always try to find code cache for native modules when it compiles them, and always produce and store the code cache after compilation. The cache map is protected by a mutex and can be accessed by different threads - including the worker threads and the main thread. Hence any thread can reuse the code cache if the native module has already been compiled by another thread - in particular the cache of the bootstrappers and per_context.js will always be hit when a new thread is spun. This results in a ~6% startup overhead in the worst case (when only the main thread is launched without requiring any additional native module - it now needs to do the extra work of finding and storing caches), which balances out the recent improvements by moving the compilation to C++, but it also leads to a ~60% improvement in the best case (when a worker thread is spun and requires a lot of native modules thus hitting the cache compiled by the main thread). PR-URL: nodejs#24950 Reviewed-By: Anna Henningsen <[email protected]>

This behavior of sometimes returning the function & other times returning the code cache was removed a long time ago in the referenced PR, as evinced by the return type `MaybeLocal<Function>`. Remove these incorrect comments. Refs: nodejs#24950

This behavior of sometimes returning the function & other times returning the code cache was removed a long time ago in the referenced PR, as evinced by the return type `MaybeLocal<Function>`. Remove these incorrect comments. Refs: #24950 PR-URL: #47083 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

joyeecheung added 2 commits December 11, 2018 07:31

src: remove code cache integrity check

216d572

In preparation of sharing code cache among different threads - we simply rely on v8 to reject invalid cache, since there isn't any serious consequence when the cache is invalid anyway.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 10, 2018

addaleax approved these changes Dec 11, 2018

View reviewed changes

src/node_native_module.cc Show resolved Hide resolved

src/node_native_module.cc Show resolved Hide resolved

src/node_native_module.cc Outdated Show resolved Hide resolved

joyeecheung force-pushed the cache-worker branch from 022a0b3 to fe44133 Compare December 11, 2018 00:12

fixup! src: always compile and store code cache for native modules

bdf589b

joyeecheung force-pushed the cache-worker branch from fe44133 to bdf589b Compare December 11, 2018 00:18

fixup! fixup! src: always compile and store code cache for native mod…

e473bb4

…ules

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2018

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2018

joyeecheung closed this Dec 18, 2018

MylesBorins added the backport-requested-v11.x label Dec 25, 2018

addaleax removed the backport-requested-v11.x label Jan 14, 2019

BridgeAR mentioned this pull request Jan 16, 2019

v11.7.0 proposal #25537

Merged

MylesBorins mentioned this pull request Jan 24, 2019

v11.8.0 proposal #25687

Merged

mmarchini mentioned this pull request Apr 22, 2019

Latest Node.js crashes when ran with --interpreted-frames-native-stack #27346

Closed

BethGriggs added the backport-requested-v10.x label Jul 2, 2019

kvakil mentioned this pull request Mar 14, 2023

src: remove dead comments about return_code_cache #47083

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: always compile and store code cache for native modules #24950

src: always compile and store code cache for native modules #24950

joyeecheung commented Dec 10, 2018

nodejs-github-bot commented Dec 10, 2018

joyeecheung commented Dec 10, 2018

joyeecheung commented Dec 11, 2018

joyeecheung commented Dec 12, 2018 •

edited

Loading

BridgeAR commented Dec 12, 2018

joyeecheung commented Dec 12, 2018

BridgeAR commented Dec 13, 2018

joyeecheung commented Dec 18, 2018

joyeecheung commented Dec 18, 2018

MylesBorins commented Dec 25, 2018

BridgeAR commented Jan 9, 2019

addaleax commented Jan 14, 2019

src: always compile and store code cache for native modules #24950

src: always compile and store code cache for native modules #24950

Conversation

joyeecheung commented Dec 10, 2018

src: remove code cache integrity check

src: always compile and store code cache for native modules

Checklist

nodejs-github-bot commented Dec 10, 2018

joyeecheung commented Dec 10, 2018

joyeecheung commented Dec 11, 2018

joyeecheung commented Dec 12, 2018 • edited Loading

BridgeAR commented Dec 12, 2018

joyeecheung commented Dec 12, 2018

BridgeAR commented Dec 13, 2018

joyeecheung commented Dec 18, 2018

joyeecheung commented Dec 18, 2018

MylesBorins commented Dec 25, 2018

BridgeAR commented Jan 9, 2019

addaleax commented Jan 14, 2019

joyeecheung commented Dec 12, 2018 •

edited

Loading