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 { 
        
           class LeftTrimmerVerifierRootVisitor : public RootVisitor { 
        
            public: 
        
             explicit LeftTrimmerVerifierRootVisitor(FixedArrayBase* to_check) 
        
                 : to_check_(to_check) {} 
        
             virtual void VisitRootPointers(Root root, const char* 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

@@ -1134,121 +1134,7 @@
      }],
    ],  # conditions
    'configurations': {
-      # Abstract configuration for v8_optimized_debug == 0.


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`.

refack · 2018-10-31T16:13:09Z

Landed in 56881b0...df7f629

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #23704 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #23704 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #23704 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

PR-URL: #23704 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 803fce2 Compare 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 b4c6b16 Compare 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:master Oct 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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

nodejs-github-bot commented Oct 16, 2018

refack commented Oct 16, 2018

addaleax commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

addaleax commented Oct 17, 2018

refack commented Oct 17, 2018

hashseed commented Oct 18, 2018

refack commented Oct 18, 2018

joyeecheung left a comment

joyeecheung Oct 19, 2018

refack Oct 19, 2018

refack commented Oct 19, 2018

addaleax commented Oct 20, 2018

refack commented Oct 20, 2018

refack commented Oct 31, 2018

joyeecheung left a comment •

edited

Loading

ryzokuken left a comment

refack commented Oct 31, 2018

refack commented Oct 31, 2018

MylesBorins commented Nov 27, 2018

MylesBorins commented Dec 3, 2018

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

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

Checklist

nodejs-github-bot commented Oct 16, 2018

refack commented Oct 16, 2018

addaleax commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

refack commented Oct 17, 2018

addaleax commented Oct 17, 2018

refack commented Oct 17, 2018

hashseed commented Oct 18, 2018

refack commented Oct 18, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung Oct 19, 2018

Choose a reason for hiding this comment

refack Oct 19, 2018

Choose a reason for hiding this comment

refack commented Oct 19, 2018

addaleax commented Oct 20, 2018

refack commented Oct 20, 2018

refack commented Oct 31, 2018

joyeecheung left a comment • edited Loading

Choose a reason for hiding this comment

ryzokuken left a comment

Choose a reason for hiding this comment

refack commented Oct 31, 2018

refack commented Oct 31, 2018

MylesBorins commented Nov 27, 2018

MylesBorins commented Dec 3, 2018

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