Skip to content

deps: fix wrong default for v8 handle zapping#23801

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
Merged
refack merged 1 commit into from
Oct 23, 2018
Merged

deps: fix wrong default for v8 handle zapping #23801

refack merged 1 commit into from
Oct 23, 2018
+14 −5

Conversation

@refack
Copy link
Contributor

@refackrefack 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

nodejs-github-bot commented Oct 21, 2018

@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-botnodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Oct 21, 2018
@refackrefack mentioned this pull request Oct 21, 2018
Closed
@refack
Copy link
ContributorAuthor

refack commented Oct 21, 2018

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

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

addaleax
addaleax reviewed Oct 21, 2018
View reviewed changes
@addaleax
Copy link
Member

addaleax commented Oct 21, 2018

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

addaleax commented Oct 21, 2018

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

@refackrefackforce-pushed the fix-handle-zapping branch 2 times, most recently from 80e3b52 to c460808CompareOctober 21, 2018 15:41
targos
targos reviewed Oct 21, 2018
View reviewed changes
Copy link
Member

@targostargos 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

@addaleaxaddaleax 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/

@addaleaxaddaleax dismissed their stale reviewOctober 21, 2018 16:40

doesn’t seem to work for Debug builds yet

@refack
Copy link
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

refack commented Oct 21, 2018

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

@refack
Copy link
ContributorAuthor

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
ContributorAuthor

refack commented Oct 21, 2018

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

@refackrefack self-assigned this Oct 22, 2018
@refackrefack mentioned this pull request Oct 22, 2018
Closed
addaleax
addaleax approved these changes Oct 23, 2018
View reviewed changes
Copy link
Member

@addaleaxaddaleax 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
ContributorAuthor

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
ContributorAuthor

refack commented Oct 23, 2018

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

@targostargos mentioned this pull request Oct 23, 2018
Merged
@mmarchinimmarchini 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#23801Fixes: nodejs#23796 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@refackrefack merged commit c9b49a6 into nodejs:masterOct 23, 2018
@refackrefack 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: #23801Fixes: #23796 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@refackrefack 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: #23801Fixes: #23796 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matheus Marchini <[email protected]>
@targostargos mentioned this pull request Oct 27, 2018
Merged
This was referenced Nov 2, 2018
Closed
Closed
Closed
Closed
@littledivylittledivy mentioned this pull request Feb 11, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@targostargostargos approved these changes

+1 more reviewer

@mmarchinimmarchinimmarchini approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.v8 engineIssues 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