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.

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

build: for debug mode set v8_optimized_debug #23704

Merged
merged 2 commits into from
Oct 31, 2018

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 16, 2018

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

@refack 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 refack self-assigned this Oct 16, 2018
@refack refack requested a review from targos October 16, 2018 22:32
@refack refack requested a review from addaleax October 16, 2018 22:32
@refack
Copy link
Contributor Author

refack commented Oct 16, 2018

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

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

@addaleax
Copy link
Member

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
Copy link
Contributor Author

refack commented Oct 17, 2018

  1. Yes only the V8 build
  2. AFAICT it -O2
  3. I'll verify
  4. 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
Copy link
Contributor Author

refack commented Oct 17, 2018

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
Copy link
Contributor Author

refack commented Oct 17, 2018

@refack
Copy link
Contributor Author

refack commented Oct 17, 2018

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

#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

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

#ifdef ENABLE_SLOW_DCHECKS

@refack refack requested a review from Trott October 17, 2018 16:20
@addaleax
Copy link
Member

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
Copy link
Contributor Author

refack commented Oct 17, 2018

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

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
Copy link
Contributor Author

refack commented Oct 18, 2018

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

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tl;dr if I flattened the inheritance structure. form:

}, # 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
Copy link
Contributor Author

refack commented Oct 19, 2018

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

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

@refack
Copy link
Contributor Author

refack commented Oct 20, 2018

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 refack force-pushed the default-v8-optimized-debug branch from 693c7bb to 803fce2 Compare October 31, 2018 02:10
@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

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

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo a more detailed commit message in the first commit

Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@refack refack force-pushed the default-v8-optimized-debug branch from 803fce2 to b4c6b16 Compare October 31, 2018 14:31
@refack
Copy link
Contributor Author

refack commented Oct 31, 2018

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 refack removed their assignment Oct 31, 2018
@refack refack merged commit df7f629 into nodejs:master Oct 31, 2018
@refack 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
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>
targos pushed a commit that referenced this pull request Nov 2, 2018
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 27, 2018
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>
MylesBorins pushed a commit that referenced this pull request Nov 27, 2018
PR-URL: #23704
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@MylesBorins
Copy link
Contributor

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

@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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
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>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
PR-URL: #23704
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
@MylesBorins
Copy link
Contributor

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]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants