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

src: implement per-process native Debug() printer and use it in mkcodecache #31884

Closed
wants to merge 7 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 20, 2020

src: make aliased_buffer.h self-contained

aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

src: refactor debug category parsing

Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

src: implement per-process native Debug() printer

This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

tools: use per-process native Debug() printer in mkcodecache

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.
@joyeecheung joyeecheung added c++ Issues and PRs that require attention from people who are familiar with C++. tools Issues and PRs related to the tools directory. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 20, 2020
src/debug_utils.h Outdated Show resolved Hide resolved
src/env.h Outdated
bool debug_enabled_[static_cast<int>(DebugCategory::CATEGORY_COUNT)] = {
false};

std::unique_ptr<EnabledDebugList> enabled_debug_list_;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason that this would need to be a pointer? I don’t think that’s necessary, and it comes with one additional layer of indirection for each debug check. One of the goals for the Debug() implementation was to have as close to zero overhead as possible, and I think this might get in the way of it…

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly to avoid the circular dependency - but I guess this can also be achieved by moving more things from debug_utils.h to debug_utils-inl.h so that env.h can just include debug_utils.h and debug_utils.h do not need to include env.h ?

Copy link
Member

Choose a reason for hiding this comment

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

If that works, then that would be great – otherwise we might need to move more things into env.h

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, looks like if we want to keep the initialization in the constructor, then it needs to be a pointer since we'd need the environment pointer with initialized env_vars for credentials::SafeGetenv(). So I just moved the initialization into EnableDebugList::Parse(), and call it in InitializeOncePerProcess (and separately, in mkcodecache since it does not need InitializeOncePerProcess()), this also means that for now the printer is not usable until node::Start() is called in the default binary, but I guess that's fine, compared to reordering the whole initialization sequence to work around pointers.

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, what I had in mind is to use the trivial copy constructor to re-initialize it later, e.g. enabled_debug_list_ = EnabledDebugList(...);, but I guess ::Parse() works just as well :)

src/node_env_var.cc Outdated Show resolved Hide resolved
@@ -1225,6 +1225,16 @@
],

'conditions': [
[ 'node_use_openssl=="true"', {
Copy link
Member Author

@joyeecheung joyeecheung Feb 25, 2020

Choose a reason for hiding this comment

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

Just realized that these are necessary for the headers (esp. enums) to match in mkcodecache and node_mksnapshot binaries - hopefully all these cruft can be cleaned up when #31074 is addressed, though the last time I tried SmartOS wouldn't link, sigh.

joyeecheung added a commit that referenced this pull request Mar 10, 2020
aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
joyeecheung added a commit that referenced this pull request Mar 10, 2020
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
joyeecheung added a commit that referenced this pull request Mar 10, 2020
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
joyeecheung added a commit that referenced this pull request Mar 10, 2020
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 10, 2020

Landed in bb6125b...192cb72, thanks!

For posterity - if you encounter linking issues after this patch please re-run configure as this patch has changed the build flags of mkcodecache and mksnapshot

MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
codebytere pushed a commit that referenced this pull request Mar 23, 2020
aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 23, 2020
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 23, 2020
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 23, 2020
@codebytere codebytere mentioned this pull request Mar 24, 2020
codebytere pushed a commit that referenced this pull request Mar 30, 2020
aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
codebytere pushed a commit that referenced this pull request Mar 31, 2020
aliased_buffer.h uses MultiplyWithOverflowCheck() implemented
in util-inl.h

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 31, 2020
Move the debug category parsing into a new EnabledDebugList class,
so that it can be reused outside of Environment.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 31, 2020
This patch adds a per-process native Debug() printer that can be
called when an Environment is not available.

PR-URL: #31884
Reviewed-By: Anna Henningsen <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants