★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/23801Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Reviewers

@addaleax addaleax

@targos targos

@mmarchini mmarchini

Assignees
No one assigned
Labels
V8 Engine author ready land-on-v10.x
Projects
None yet
Milestone
No milestone
6 participants
@refack @nodejs-github-bot @addaleax @targos @mmarchini @MylesBorins
@refack
Copy link
Member

refack commented Oct 21, 2018 •
edited

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 nodejs-github-bot added the V8 Engine label Oct 21, 2018

@refack refack referenced this pull request Oct 21, 2018

Closed

V8 gyp file refactor performance regression #23796

@refack

This comment has been minimized.

Sign in to view
Copy link
Member 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

addaleax reviewed Oct 21, 2018
View changes

Show resolved Hide resolved deps/v8/gypfiles/features.gypi Outdated
@addaleax

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Oct 21, 2018

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

@refack refack force-pushed the refack:fix-handle-zapping branch 2 times, most recently from 80e3b52 to c460808 Oct 21, 2018

@targos

targos reviewed Oct 21, 2018
View changes

Copy link
Member

targos left a comment

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

addaleax reviewed Oct 21, 2018
View changes

Copy link
Member

addaleax left a comment

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

@addaleax addaleax dismissed their stale review Oct 21, 2018

doesn’t seem to work for Debug builds yet

@refack

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 21, 2018 •
edited

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 21, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member 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

This comment has been minimized.

Sign in to view
Copy link
Member 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 referenced this pull request Oct 22, 2018

Closed

Investigate flaky test-fs-readfile-tostring-fail #16601

@addaleax

addaleax approved these changes Oct 23, 2018
View changes

Copy link
Member

addaleax left a comment

👍
@refack

This comment has been minimized.

Sign in to view
Copy link
Member 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)
@targos

targos approved these changes Oct 23, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member Author

refack commented Oct 23, 2018

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

@targos targos referenced this pull request Oct 23, 2018

Merged

2018-10-30 Version 10.13.0 'Dubnium' (LTS) #23831

@mmarchini

mmarchini approved these changes Oct 23, 2018
View changes

@mmarchini mmarchini added the author ready label Oct 23, 2018

@refack
deps: fix wrong default for v8 handle zapping 
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>
Loading status checks…
c9b49a6

@refack refack force-pushed the refack:fix-handle-zapping branch from 86b66a9 to c9b49a6 Oct 23, 2018

@refack refack merged commit c9b49a6 into nodejs:master Oct 23, 2018

@refack refack deleted the refack:fix-handle-zapping branch Oct 23, 2018

targos added a commit that referenced this pull request Oct 24, 2018

@refack @targos
deps: fix wrong default for v8 handle zapping 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
3b66a8d

@refack refack removed their assignment Oct 24, 2018

@MylesBorins MylesBorins added the land-on-v10.x label Oct 25, 2018

MylesBorins added a commit that referenced this pull request Oct 25, 2018

@refack @MylesBorins
deps: fix wrong default for v8 handle zapping 
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>
Verified
This commit was signed with a verified signature.
@MylesBorins MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
1003f4c

@targos targos referenced this pull request Oct 27, 2018

Merged

Release proposal: v11.1.0 #23922

This was referenced Nov 2, 2018

Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.