[canary] GYP v8 embedded builtins #22920
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
build
targos
force-pushed
the
gyp-v8-embedded-builtins
branch
from
b55c5bb to
ef1ca13
Compare
|
I updated CI: https://ci.nodejs.org/job/node-test-pull-request/17273/ |
addaleax
approved these changes
Sep 18, 2018
|
There's a problem somewhere with the generated files. It keeps recreating the |
Yes, and it seems very similar to #22006. I'm investigating... |
ivz-dev
approved these changes
Sep 18, 2018
refack
force-pushed
the
gyp-v8-embedded-builtins
branch
from
67ca583 to
0f6d4d3
Compare
|
Some more tweaks https://ci.nodejs.org/job/node-test-commit/21635/ |
|
|
|
|
|
Hundreds of lines like this one: @nodejs/platform-aix |
|
AFAICT there is still a dependency race I need to fix (code looking for generated files that have not been generated yet). /CC @nodejs/v8 |
Is it possible to get hold of the |
|
@richardlau command line (interesting bits in the middle) |
|
One thought that comes to mind is: Do we know if upstream V8 are using gas on AIX instead of the AIX assembler? On the Node.js CI we're definitely using the AIX assembler, based on the I am assuming that this all works upstream. cc @john-yan (since he was pinged in nodejs/node-v8#79) |
refack
force-pushed
the
gyp-v8-embedded-builtins
branch
from
a6069bb to
278d205
Compare
refack
added
the
author ready
refack
commented
Oct 4, 2018
|
|
||
| 'v8_perf_prof_unwinding_info%': 0, | ||
|
|
||
| 'v8_enable_fast_mksnapshot%': 0, |
There was a problem hiding this comment.
@nodejs/v8-update do we want to turn any of these for node by default?
There was a problem hiding this comment.
Generally it makes sense to copy the settings from BUILD.gn, so
- Disable pointer compression. That's work-in-progress.
- Enable embedded builtins. I think that's the point of this PR. You save quite a lot of memory per isolate this way. But that does not work for ia32 yet.
- Disable handle zapping. That's for debugging, and should not be used in production.
- Disable untrusted code mitigations. That's to address Spectre, but that's outside Node.js' threat model.
- Disable fast mksnapshot. That's only to make mksnapshot run faster, giving a shorter turnaround during development.
There was a problem hiding this comment.
Btw untrusted code mitigations are actually disabled from common.gypi already.
targos
reviewed
Oct 4, 2018
deps/v8/gypfiles/v8.gyp
Outdated
| '../src/torque/types.h', | ||
| '../src/torque/utils.cc', | ||
| '../src/torque/utils.h', | ||
| "../src/torque/ast.h", |
There was a problem hiding this comment.
any reason for switching to double quotes?
targos
reviewed
Oct 4, 2018
targos
reviewed
Oct 4, 2018
| @@ -481,7 +481,7 @@ build-ci: | |||
| # - node-test-commit-linux-coverage: where the build and the tests need | |||
| # to be instrumented, see `coverage`. | |||
| run-ci: build-ci | |||
| $(MAKE) test-ci | |||
| $(MAKE) test-ci -j1 | |||
There was a problem hiding this comment.
No, because of #22006
|
This looks very good to me overall! |
richardlau
reviewed
Oct 4, 2018
richardlau
reviewed
Oct 4, 2018
deps/v8/gypfiles/features.gypi
Outdated
| @@ -113,6 +113,13 @@ | |||
|
|
|||
| 'v8_enable_fast_mksnapshot%': 0, | |||
| }, | |||
| 'conditions': [ | |||
| ['(OS=="aix") or (OS=="win" and (clang!=1 or v8_target_arch=="ia32"))', { | |||
There was a problem hiding this comment.
Should embedded builtins only be disabled for 32-bit Intel on Windows only or on all 32-bit Intel platforms? (I know we only support 32-bit Intel on Windows for Node.js but thinking of others).
There was a problem hiding this comment.
I'll replicate the predicate in BUILD.gn:
https://github.com/nodejs/node/blob/ae461f52330710858f1282bfacdd2da6e0145dd9/deps/v8/BUILD.gn#L74-L77
There was a problem hiding this comment.
For reference, the condition in BUILD.gn:
# Enable embedded builtins.
# TODO(jgruber,v8:6666): Support ia32 and maybe MSVC.
v8_enable_embedded_builtins = v8_use_snapshot && v8_current_cpu != "x86" &&
!is_aix && (!is_win || is_clang)
This comment has been minimized.
This comment has been minimized.
refack
force-pushed
the
gyp-v8-embedded-builtins
branch
from
3a80db9 to
0dc2c7d
Compare
This comment has been minimized.
This comment has been minimized.
enable v8_enable_embedded_builtins reorder conditions proccessing for `run_mksnapshot`
refack
force-pushed
the
gyp-v8-embedded-builtins
branch
from
0dc2c7d to
b65f4d2
Compare
This was referenced Oct 5, 2018
refack
removed
the
author ready
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesRoughly working. In CI I've stumbled into #22006 on more platforms, probably because the dep-chain changed.
Would live some review @nodejs/gyp @nodejs/python @nodejs/build-files @nodejs/v8-update
Fixes: nodejs/node-v8#75
Fixes: nodejs/node-v8#39
Fixes: nodejs/node-v8#74
Fixes: #22757