build: for debug mode set `v8_optimized_debug`#23704

refack · 2018-10-16T22:32:04Z

This cuts down the debug CI run from ~1:20h to 50m

Second commit is a bugfix in the V8 gypfiles. Since common.gypi already defines target_defaults, the previous setup in gypfiles/toolchain.gypi (with setting inheritance) simply didn't work. A partial comparison of the results of the two state of this flag are available at https://www.diffchecker.com/Q340bUuJ

Under the assumption that debugging is more often focused on node core source.
This setting still compiles V8 with partial optimizations and with debug symbols, so it is still very much debuggable, but it is much faster.
Override is configurable by ./configure --v8-non-optimized-debug

Checklist

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

nodejs-github-bot · 2018-10-16T22:32:05Z

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

refack · 2018-10-16T22:33:14Z

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

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

addaleax · 2018-10-17T05:41:54Z

So, to make sure:

This is only about the V8 part of the build, right?
It adds one of the -O flags? -Og?
It does not affect DCHECKs etc.?

I can’t really say anything about the second commit either way (and there’s no description about what is being fixed…)

refack · 2018-10-17T07:34:59Z

Yes only the V8 build
AFAICT it -O2
I'll verify
I'll elaborate on the fix. (tl;Dr with out it the flag did work at all, and V8 would be built with node's debug setting

refack · 2018-10-17T07:52:57Z

BTW this cuts down the debug CI run from ~1:20h to 50m
And I'm assuming on Windows it's closer to 50%

refack · 2018-10-17T07:53:49Z

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

refack · 2018-10-17T16:16:40Z

@addaleax Here is the diff https://www.diffchecker.com/Q340bUuJ
the flag removes -DENABLE_SLOW_DCHECKS (but keeps -DV8_ENABLE_CHECKS) and compiles with -O3.

ENABLE_SLOW_DCHECKS effects just a few places:

node/deps/v8/src/elements.cc

Line 2169 in cf3f8dd

#if ENABLE_SLOW_DCHECKS

node/deps/v8/src/heap/heap.cc

Lines 2860 to 2881 in cf3f8dd

 #ifdef ENABLE_SLOW_DCHECKS 
namespace{
classLeftTrimmerVerifierRootVisitor : publicRootVisitor{
public:
explicitLeftTrimmerVerifierRootVisitor(FixedArrayBase* to_check) 
 : to_check_(to_check){} 
virtualvoidVisitRootPointers(Root root, constchar* description, 
 Object** start, Object** end){
for (Object** p = start; p < end; ++p){
DCHECK_NE(*p, to_check_); 
 } 
 } 
private:
 FixedArrayBase* to_check_; 
DISALLOW_COPY_AND_ASSIGN(LeftTrimmerVerifierRootVisitor); 
 }; 
 } // namespace
 #endif// ENABLE_SLOW_DCHECKS

node/deps/v8/src/heap/heap.cc

Lines 2972 to 2979 in cf3f8dd

 #ifdef ENABLE_SLOW_DCHECKS 
if (FLAG_enable_slow_asserts){
// Make sure the stack or other roots (e.g., Handles) don't contain pointers
// to the original FixedArray (which is now the filler object).
 LeftTrimmerVerifierRootVisitor root_visitor(object); 
IterateRoots(&root_visitor, VISIT_ALL); 
 } 
 #endif// ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 2603 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 2683 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 9327 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 11703 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 11754 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/objects.cc

Line 15638 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/source-position-table.cc

Line 114 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/source-position-table.cc

Line 150 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/source-position-table.cc

Line 164 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/src/source-position-table.cc

Line 181 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/test/cctest/compiler/test-code-generator.cc

Line 677 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

node/deps/v8/test/cctest/compiler/test-code-generator.cc

Line 686 in cf3f8dd

#ifdef ENABLE_SLOW_DCHECKS

addaleax · 2018-10-17T19:41:48Z

I’m not sure how I feel about ENABLE_SLOW_DCHECKS going away… could we define that manually? I have no idea how much slower it actually is, but at least in CI we’d like to have all the checks we can get, I think…?

refack · 2018-10-17T20:45:01Z

the gypfiles are under our responsibility, so defining it is no problem, but I my default is to trust the V8 team's preferences.

Maybe someone from @nodejs/v8 can give perspective on the importance of ENABLE_SLOW_DCHECKS

hashseed · 2018-10-18T05:40:01Z

SLOW_DCHECKS are mostly used for expensive consistency checks. I don't think you will catch many issues with this on CI, since most of these places are deep inside of V8. It might be nice to have a way to enable it though, to debug tricky issues.

refack · 2018-10-18T15:26:04Z

It might be nice to have a way to enable it though, to debug tricky issues.

As they say, that's an excellent point (flips to next slide) ./configure --v8-non-optimized-debug

joyeecheung

I assume we can still use https://ci.nodejs.org/job/node-test-commit-custom-suites/ (with CONFIG_FLAGS=--non-optimized-debug) to try the slow checks in the CI after this?

joyeecheung · 2018-10-19T08:42:29Z

deps/v8/gypfiles/toolchain.gypi

If I am reading this correctly, this switches the configuration-based conditional to a variable-based conditional (v8_optimized_debug) to fix the bug? Can you paste the PR message into the commit message of the second commit, since it is a bit difficult to tell what the diff is doing due to all the indentations?

the tl;dr if I flattened the inheritance structure. form:
node/deps/v8/gypfiles/toolchain.gypi
Lines 1315 to 1325 in 9800fd7
}, # DebugBaseCommon
'Debug':{
'inherit_from': ['DebugBaseCommon'],
'conditions': [
['v8_optimized_debug==0',{
'inherit_from': ['DebugBase0'],
},{
'inherit_from': ['DebugBase1'],
}],
],
}, # Debug

to
https://github.com/nodejs/node/blob/693c7bbe621f4ab8686397e301e1f8027e734dff/deps/v8/gypfiles/toolchain.gypi#L1136-L1137
...
https://github.com/nodejs/node/blob/693c7bbe621f4ab8686397e301e1f8027e734dff/deps/v8/gypfiles/toolchain.gypi#L1200
I'll document that

refack · 2018-10-19T13:29:44Z

I assume we can still use ci.nodejs.org/job/node-test-commit-custom-suites (with CONFIG_FLAGS=--non-optimized-debug) to try the slow checks in the CI after this?

https://ci.nodejs.org/job/node-test-commit-custom-suites/727/default/

09:28:22 + make build-ci -j 4 09:28:22 python ./configure --verbose --v8-non-optimized-debug 09:28:23 creating icu_config.gypi ... 09:28:23 'v8_optimized_debug': 0,

Verified ✔️

addaleax · 2018-10-20T04:09:53Z

@refack With this, would we even need 74c4bb7 anymore?

refack · 2018-10-20T13:29:05Z

With this, would we even need 74c4bb7 anymore?

My goal is to get node-test-linux-linked-debug to consistently run < 60m.
If lands, I'll re-evaluate the list from 74c4bb7.

refack · 2018-10-31T02:12:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18250/
/CC @nodejs/v8-update @nodejs/build-files PTAL

joyeecheung

LGTM modulo a more detailed commit message in the first commit

ryzokuken

LGTM given that we can still enable slow DCHECKs whenever required.

Thanks, especially for all the work on the gypfiles, @refack.

refack · 2018-10-31T14:33:14Z

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

modulo a more detailed commit message in the first commit

@joyeecheung this is what I came up with:

@refack build: configure default v8_optimized_debug Under the assumption that debugging is more often focused on node core source. This setting compiles V8 with only partial optimizations, DCHECKS, and debug symbols, so it is still very much debuggable, but it is much faster. It does disable SLOW_DCHECKS, but at the advice of the V8 team, those are more important for deep V8 debugging. Override is configurable with `./configure --v8-non-optimized-debug`.

Under the assumption that debugging is more often focused on node core source. This setting compiles V8 with only partial optimizations, DCHECKS, and debug symbols, so it is still very much debuggable, but it is much faster. It does disable SLOW_DCHECKS, but at the advice of the V8 team, those are more important for deep V8 debugging. Override is configurable with `./configure --v8-non-optimized-debug`. PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

Under the assumption that debugging is more often focused on node core source. This setting compiles V8 with only partial optimizations, DCHECKS, and debug symbols, so it is still very much debuggable, but it is much faster. It does disable SLOW_DCHECKS, but at the advice of the V8 team, those are more important for deep V8 debugging. Override is configurable with `./configure --v8-non-optimized-debug`. PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

MylesBorins · 2018-11-27T07:05:34Z

I've opted to land on 10.x but not on 8.x, it landed with minimal changes

Feel free to open a backport if you like

Under the assumption that debugging is more often focused on node core source. This setting compiles V8 with only partial optimizations, DCHECKS, and debug symbols, so it is still very much debuggable, but it is much faster. It does disable SLOW_DCHECKS, but at the advice of the V8 team, those are more important for deep V8 debugging. Override is configurable with `./configure --v8-non-optimized-debug`. PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

Under the assumption that debugging is more often focused on node core source. This setting compiles V8 with only partial optimizations, DCHECKS, and debug symbols, so it is still very much debuggable, but it is much faster. It does disable SLOW_DCHECKS, but at the advice of the V8 team, those are more important for deep V8 debugging. Override is configurable with `./configure --v8-non-optimized-debug`. PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

MylesBorins · 2018-12-03T18:34:32Z

These changes unfortunately break the build on v10.x for windows. Below is an example of the error. Would someone be willing to manually backport?

12:12:48 ..\third_party\antlr4\runtime\Cpp\runtime\src\atn\ATNSerializer.cpp(288): error C2280: 'std::shared_ptr<antlr4::atn::LexerChannelAction> std::dynamic_pointer_cast<antlr4::atn::LexerChannelAction,antlr4::atn::LexerAction>(const std::shared_ptr<antlr4::atn::LexerAction> &) noexcept': attempting to reference a deleted function [c:\workspace\node-compile-windows\deps\v8\gypfiles\torque.vcxproj]

refack added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Oct 16, 2018

refack self-assigned this Oct 16, 2018

refack requested a review from targos October 16, 2018 22:32

refack requested a review from addaleax October 16, 2018 22:32

refack requested a review from Trott October 17, 2018 16:20

joyeecheung reviewed Oct 19, 2018
View reviewed changes

refack mentioned this pull request Oct 21, 2018
deps: fix wrong default for v8 handle zapping #23801
Merged
3 tasks

refack force-pushed the default-v8-optimized-debug branch from 693c7bb to 803fce2Compare October 31, 2018 02:10

joyeecheung approved these changes Oct 31, 2018
View reviewed changes

ryzokuken approved these changes Oct 31, 2018
View reviewed changes

refack force-pushed the default-v8-optimized-debug branch from 803fce2 to b4c6b16Compare October 31, 2018 14:31

refack removed request for Trott, addaleax and targos October 31, 2018 16:13

refack removed their assignment Oct 31, 2018

refack merged commit df7f629 into nodejs:masterOct 31, 2018

refack deleted the default-v8-optimized-debug branch November 1, 2018 02:28

targos pushed a commit that referenced this pull request Nov 2, 2018
deps,v8: fix gypfile bug
ef8b5b4
PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

BridgeAR mentioned this pull request Nov 14, 2018
Release proposal: v11.2 #24350
Merged

MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
deps,v8: fix gypfile bug
67f5d30
PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

MylesBorins added dont-land-on-v8.x labels Nov 27, 2018

codebytere mentioned this pull request Nov 27, 2018
v10.13.1 proposal #24675
Closed

rvagg pushed a commit that referenced this pull request Nov 28, 2018
deps,v8: fix gypfile bug
0c70838
PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
deps,v8: fix gypfile bug
c9ecaef
PR-URL: #23704 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]>

codebytere mentioned this pull request Nov 29, 2018
v10.14.2 proposal #24727
Merged

MylesBorins added backport-requested-v10.x and removed land-on-v10.x labels Dec 3, 2018

joyeecheung mentioned this pull request Oct 9, 2024
[rfc] V8 debug build CI? #55326
Open

	}, # DebugBaseCommon
	'Debug':{
	'inherit_from': ['DebugBaseCommon'],
	'conditions': [
	['v8_optimized_debug==0',{
	'inherit_from': ['DebugBase0'],
	},{
	'inherit_from': ['DebugBase1'],
	}],
	],
	}, # Debug

Uh oh!

build: for debug mode set v8_optimized_debug#23704

build: for debug mode set v8_optimized_debug#23704

Conversation

refack commented Oct 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Oct 16, 2018

Uh oh!

refack commented Oct 16, 2018

Uh oh!

addaleax commented Oct 17, 2018

Uh oh!

refack commented Oct 17, 2018

Uh oh!

refack commented Oct 17, 2018

Uh oh!

refack commented Oct 17, 2018

Uh oh!

refack commented Oct 17, 2018

Uh oh!

addaleax commented Oct 17, 2018

Uh oh!

refack commented Oct 17, 2018

Uh oh!

hashseed commented Oct 18, 2018

Uh oh!

refack commented Oct 18, 2018

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheungOct 19, 2018

Choose a reason for hiding this comment

Uh oh!

refackOct 19, 2018

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 19, 2018

Uh oh!

addaleax commented Oct 20, 2018

Uh oh!

refack commented Oct 20, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

joyeecheung left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Oct 31, 2018

Uh oh!

MylesBorins commented Nov 27, 2018

Uh oh!

MylesBorins commented Dec 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build: for debug mode set `v8_optimized_debug`#23704

build: for debug mode set `v8_optimized_debug`#23704

refack commented Oct 16, 2018•
edited
Loading

joyeecheung left a comment •
edited
Loading