[v11.x backport] src: simplify NativeModule caching and remove redundant data #25964

antsmartian · 2019-02-06T16:45:16Z

Backport: #25352

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

- Remove `NativeModule._source` - the compilation is now entirely done in C++ and `process.binding('natives')` is implemented directly in the binding loader so there is no need to store additional source code strings. - Instead of using an object as `NativeModule._cached` and insert into it after compilation of each native module, simply prebuild a JS map filled with all the native modules and infer the state of compilation through `mod.loading`/`mod.loaded`. - Rename `NativeModule.nonInternalExists` to `NativeModule.canBeRequiredByUsers` and precompute that property for all the native modules during bootstrap instead of branching in every require call during runtime. This also fixes the bug where `worker_threads` can be made available with `--expose-internals`. - Rename `NativeModule.requireForDeps` to `NativeModule.requireWithFallbackInDeps`. - Add a test to make sure we do not accidentally leak any module to the global namespace. PR-URL: nodejs#25352 Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot · 2019-02-06T16:45:20Z

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

addaleax · 2019-02-06T16:48:23Z

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

addaleax · 2019-02-06T18:29:23Z

I’ve added a fixup commit because it looks like this breaks a test? @joyeecheung is this is an okay way to address this?

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

antsmartian · 2019-02-07T05:04:39Z

@addaleax Yes, looks it broke a test.

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

addaleax · 2019-02-07T06:19:14Z

@nodejs/build-infra Could you take a look at the ARM failure?

richardlau · 2019-02-07T06:44:54Z

@nodejs/build-infra Could you take a look at the ARM failure?

If it helps:

Failure is:

00:10:20 make test-ci -j1
00:10:20 make[1]: warning: -jN forced in submake: disabling jobserver mode.
00:10:21 Clean up any leftover processes but don't error if found.
00:10:21 ps awwx | grep Release/node | grep -v grep | cat
00:10:21 65286 pts/1    Rl     0:23 out/Release/node /var/tmp/shigeki/node/test/parallel/test-http2-forget-closed-streams.js
00:10:21 70826 pts/1    Sl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-policy-integrity.js
00:10:21 72011 pts/1    Sl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stdio-pipe-access.js
00:10:21 73034 pts/1    Rl     0:01 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stringbytes-external.js
00:10:21 73090 pts/1    Rl     0:01 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stream2-large-read-stall.js
00:10:21 73126 pts/1    Sl     0:01 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stream2-read-sync-stack.js
00:10:21 73341 pts/1    Sl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-sync-io-option.js
00:10:21 73369 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stream2-httpclient-response-end.js
00:10:21 73376 pts/1    Sl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stdio-pipe-access.js parent
00:10:21 73377 pts/1    Sl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-tick-processor-arguments.js
00:10:21 73443 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timer-immediate.js
00:10:21 73446 pts/1    Rl     0:00 /var/tmp/shigeki/node/out/Release/node /var/tmp/shigeki/node/test2.js test2.log
00:10:21 73448 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-socket-timeout-removes-other-socket-unref-timer.js
00:10:21 73450 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-this.js
00:10:21 73455 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-timeout-with-non-integer.js
00:10:21 73468 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-unref.js
00:10:21 73469 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stream2-set-encoding.js
00:10:21 73494 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-immediate.js
00:10:21 73496 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-unref-remove-other-unref-timers-only-one-fires.js
00:10:21 73502 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-unref-throw-then-ref.js
00:10:21 73516 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-immediate-queue-throw.js
00:10:21 73526 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-unrefd-interval-still-fires.js
00:10:21 73536 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-immediate-unref.js
00:10:21 73542 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-stream3-cork-uncork.js
00:10:21 73543 pts/1    Rl     0:00 /var/tmp/shigeki/node/out/Release/node /var/tmp/shigeki/node/test2.js test2.log
00:10:21 73555 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-unrefed-in-beforeexit.js
00:10:21 73574 pts/1    Rl     0:00 out/Release/node /var/tmp/shigeki/node/test/parallel/test-timers-immediate-unref-nested-once.js
00:10:21 Makefile:449: recipe for target 'clear-stalled' failed
00:10:21 make[1]: *** [clear-stalled] Error 123
00:10:21 Makefile:533: recipe for target 'run-ci' failed
00:10:21 make: *** [run-ci] Error 2
00:10:22 Build step 'Conditional steps (multiple)' marked build as failure

Looks like it failed in (although the line number puts it on the @echo which is odd as clearly the ps was executed -- most likely it failed during xargs kill -9?):

node/Makefile

Lines 447 to 454 in c5a7fed

    
           .PHONY: clear-stalled 
        
           clear-stalled: 
        
           	@echo "Clean up any leftover processes but don't error if found." 
        
           	ps awwx | grep Release/node | grep -v grep | cat 
        
           	@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \ 
        
           	if [ "$${PS_OUT}" ]; then \ 
        
           		echo $${PS_OUT} | xargs kill -9; \ 
        
           	fi

joyeecheung · 2019-02-07T16:22:15Z

@addaleax The fixup LGTM, I think it was caused by end-of-life of requiring v8 scripts but that’s not on v11

refack · 2019-02-07T16:40:09Z

Resume: https://ci.nodejs.org/job/node-test-commit/25689/

refack · 2019-02-07T16:41:14Z

nodejs/build-infra Could you take a look at the ARM failure?

FTR: failure was because manual testing were performed concomitantly with the CI job.

addaleax · 2019-02-07T17:48:02Z

Resume CI again: https://ci.nodejs.org/job/node-test-commit/25690/

addaleax · 2019-02-07T17:53:15Z

@refack It looks like that’s still ongoing … should we maybe just ignore the result for that machine? This is only a backport PR anyway. :/

addaleax · 2019-02-08T13:27:26Z

Resume CI again: https://ci.nodejs.org/job/node-test-commit/25723/

addaleax · 2019-02-08T23:38:19Z

Again: https://ci.nodejs.org/job/node-test-commit/25738/

addaleax · 2019-02-09T00:28:37Z

Landed in b280d90, thanks for the backport!

- Remove `NativeModule._source` - the compilation is now entirely done in C++ and `process.binding('natives')` is implemented directly in the binding loader so there is no need to store additional source code strings. - Instead of using an object as `NativeModule._cached` and insert into it after compilation of each native module, simply prebuild a JS map filled with all the native modules and infer the state of compilation through `mod.loading`/`mod.loaded`. - Rename `NativeModule.nonInternalExists` to `NativeModule.canBeRequiredByUsers` and precompute that property for all the native modules during bootstrap instead of branching in every require call during runtime. This also fixes the bug where `worker_threads` can be made available with `--expose-internals`. - Rename `NativeModule.requireForDeps` to `NativeModule.requireWithFallbackInDeps`. - Add a test to make sure we do not accidentally leak any module to the global namespace. Backport-PR-URL: #25964 PR-URL: #25352 Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v11.x labels Feb 6, 2019

addaleax approved these changes Feb 6, 2019

View reviewed changes

fixup! src: simplify NativeModule caching and remove redundant data

c5a7fed

addaleax closed this Feb 9, 2019

antsmartian deleted the backport-25352-11x branch February 10, 2019 06:14

Uh oh!

[v11.x backport] src: simplify NativeModule caching and remove redundant data #25964

[v11.x backport] src: simplify NativeModule caching and remove redundant data #25964

Uh oh!

Conversation

antsmartian commented Feb 6, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 6, 2019

Uh oh!

addaleax commented Feb 6, 2019

Uh oh!

addaleax commented Feb 6, 2019

Uh oh!

antsmartian commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

richardlau commented Feb 7, 2019

Uh oh!

joyeecheung commented Feb 7, 2019

Uh oh!

refack commented Feb 7, 2019

Uh oh!

refack commented Feb 7, 2019

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

addaleax commented Feb 7, 2019

Uh oh!

addaleax commented Feb 8, 2019

Uh oh!

addaleax commented Feb 8, 2019

Uh oh!

addaleax commented Feb 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

antsmartian commented Feb 7, 2019 •

edited

Loading