Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: fix wrong default for v8 handle zapping #23801

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 21, 2018

Can we devise a test for this?

Fixes: #23796

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 21, 2018
@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

/CC @nodejs/v8-update @nodejs/build-files

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

@addaleax
Copy link
Member

Can we devise a vest for this?

I think we can only catch this through benchmarking, and maybe manually comparing the compiler flags before/after GYP changes.

@addaleax
Copy link
Member

Also, according to #23182 (comment) I think we should bump the embedder version for GYP updates?

@refack refack force-pushed the fix-handle-zapping branch 2 times, most recently from 80e3b52 to c460808 Compare October 21, 2018 15:41
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work if we force it to 1 in common.gypi?

addaleax
addaleax previously approved these changes Oct 21, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running CI to verify that this works: https://ci.nodejs.org/job/node-test-pull-request/18029/

@addaleax addaleax dismissed their stale review October 21, 2018 16:40

doesn’t seem to work for Debug builds yet

@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

Does it work if we force it to 1 in common.gypi?

Yes, if we force it (i.e. without %) in common.gypi to config.gypi

@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

Does it work if we force it to 1 in common.gypi?

Yes, if we force it (i.e. without %) in common.gypi to config.gypi

No, I think it's because of the issue resolved by #23704

Running just debug vs regular CI: https://ci.nodejs.org/job/node-test-commit-linux-containered/8006/

@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

Sample from debug build https://ci.nodejs.org/job/node-test-commit-linux-containered/8007/nodes=ubuntu1604_sharedlibs_debug_x64/console):

  • Release target (no zapping)
ccache
g++
-o
/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Release/obj.target/v8_base/deps/v8/src/debug/debug-scopes.o
../deps/v8/src/debug/debug-scopes.cc
'-DV8_GYP_BUILD'
'-DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=0'
'-DV8_TARGET_ARCH_X64'
'-DV8_EMBEDDER_STRING="-node.6"'
'-DENABLE_DISASSEMBLER'
'-DV8_PROMISE_INTERNAL_FIELD_COUNT=1'
'-DV8_INTL_SUPPORT'
'-DV8_CONCURRENT_MARKING'
'-DDISABLE_UNTRUSTED_CODE_MITIGATIONS'
'-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC'
'-DUCONFIG_NO_SERVICE=1'
'-DU_ENABLE_DYLOAD=0'
'-DU_STATIC_IMPLEMENTATION=1'
'-DU_HAVE_STD_STRING=1'
'-DUCONFIG_NO_BREAK_ITERATION=0'
-I../deps/v8
-I../.
-I/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Release/obj/gen
-I../deps/v8/include
-I../deps/icu-small/source/i18n
-I../deps/icu-small/source/common

-pthread
-Wall
-Wextra
-Wno-unused-parameter
-m64
-fno-strict-aliasing
-m64
-fdata-sections
-ffunction-sections
-O3
-O3
-fno-omit-frame-pointer
-fno-rtti
-fno-exceptions
-std=gnu++1y
-MMD
-MF
/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Release/.deps//home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Release/obj.target/v8_base/deps/v8/src/debug/debug-scopes.o.d.raw
-c
  • Debug target (has '-DENABLE_HANDLE_ZAPPING'):
ccache
g++
-o
/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Debug/obj.target/v8_base/deps/v8/src/compiler/jump-threading.o
../deps/v8/src/compiler/jump-threading.cc
'-DV8_GYP_BUILD'
'-DV8_TYPED_ARRAY_MAX_SIZE_IN_HEAP=0'
'-DV8_TARGET_ARCH_X64'
'-DV8_EMBEDDER_STRING="-node.6"'
'-DENABLE_DISASSEMBLER'
'-DV8_PROMISE_INTERNAL_FIELD_COUNT=1'
'-DV8_INTL_SUPPORT'
'-DV8_CONCURRENT_MARKING'
'-DDISABLE_UNTRUSTED_CODE_MITIGATIONS'
'-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC'
'-DUCONFIG_NO_SERVICE=1'
'-DU_ENABLE_DYLOAD=0'
'-DU_STATIC_IMPLEMENTATION=1'
'-DU_HAVE_STD_STRING=1'
'-DUCONFIG_NO_BREAK_ITERATION=0'
'-DV8_ENABLE_CHECKS'
'-DOBJECT_PRINT'
'-DVERIFY_HEAP'
'-DDEBUG'
'-DV8_TRACE_MAPS'
'-DV8_ENABLE_ALLOCATION_TIMEOUT'
'-DV8_ENABLE_FORCE_SLOW_PATH'
'-DENABLE_SLOW_DCHECKS'
'-D_DEBUG'
'-DENABLE_HANDLE_ZAPPING'
-I../deps/v8
-I../.
-I/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Debug/obj/gen
-I../deps/v8/include
-I../deps/icu-small/source/i18n
-I../deps/icu-small/source/common
-pthread
-Wall
-Wextra
-Wno-unused-parameter
-m64
-fno-strict-aliasing
-m64
-Woverloaded-virtual
-fdata-sections
-ffunction-sections
-g
-O0
-fno-rtti
-fno-exceptions
-std=gnu++1y
-MMD
-MF
/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Debug/.deps//home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/out/Debug/obj.target/v8_base/deps/v8/src/compiler/jump-threading.o.d.raw
-c

@refack
Copy link
Contributor Author

refack commented Oct 21, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

@nodejs/v8-update @nodejs/build-files PTAL
(this solve a regression (#23796) that since #23182 we build Release binaries with -DENABLE_HANDLE_ZAPPING which has a performance penalty)

@refack
Copy link
Contributor Author

refack commented Oct 23, 2018

@mmarchini mmarchini added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2018
PR-URL: nodejs#23801
Fixes: nodejs#23796
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matheus Marchini <[email protected]>
@refack refack merged commit c9b49a6 into nodejs:master Oct 23, 2018
@refack refack deleted the fix-handle-zapping branch October 23, 2018 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V8 gyp file refactor performance regression
6 participants