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.

Sign up for GitHub

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

Jump to bottom

deps: fix wrong default for v8 handle zapping #23801

Merged
merged 1 commit into from
Oct 23, 2018
Merged

deps: fix wrong default for v8 handle zapping #23801

merged 1 commit into from
Oct 23, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 21, 2018 •
edited
Loading

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
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 21, 2018
@refack refack mentioned this pull request Oct 21, 2018
Closed
@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
targos
targos reviewed Oct 21, 2018
View reviewed changes
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
View reviewed changes
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 •
edited
Loading

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

https://ci.nodejs.org/job/node-test-commit-linux-containered/8007/

@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

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

@refack refack self-assigned this Oct 22, 2018
@refack refack mentioned this pull request Oct 22, 2018
Closed
addaleax
addaleax approved these changes Oct 23, 2018
View reviewed changes
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

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

@targos targos mentioned this pull request Oct 23, 2018
Merged
@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
@refack
deps: fix wrong default for v8 handle zapping
c9b49a6 
PR-URL: nodejs#23801
Fixes: nodejs#23796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@refack refack merged commit c9b49a6 into nodejs:master Oct 23, 2018
@refack refack deleted the fix-handle-zapping branch October 23, 2018 15:01
targos pushed a commit that referenced this pull request Oct 24, 2018
@refack @targos
deps: fix wrong default for v8 handle zapping
3b66a8d 
PR-URL: #23801
Fixes: #23796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@refack refack removed their assignment Oct 24, 2018
MylesBorins pushed a commit that referenced this pull request Oct 25, 2018
@refack @MylesBorins
deps: fix wrong default for v8 handle zapping
1003f4c 
PR-URL: #23801
Fixes: #23796
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
@targos targos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
@littledivy littledivy mentioned this pull request Feb 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmarchini mmarchini mmarchini approved these changes

@targos targos targos approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

V8 gyp file refactor performance regression
6 participants
@refack @nodejs-github-bot @addaleax @targos @mmarchini @MylesBorins