Skip to content

src: add option to report large page stats - #32331

Closed
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:large-pages-verbose
Closed

src: add option to report large page stats#32331
gabrielschulhof wants to merge 1 commit into
nodejs:masterfrom
gabrielschulhof:large-pages-verbose

Conversation

@gabrielschulhof

@gabrielschulhofgabrielschulhof commented Mar 17, 2020

Copy link
Copy Markdown
Contributor

This adds the new option verbose to --use-largepages that works the
same way as on, but additionally outputs memory range and page count
information to stderr.

Signed-off-by: @gabrielschulhof

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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 17, 2020
@nodejs-github-bot

nodejs-github-bot commented Mar 18, 2020

Copy link
Copy Markdown
Collaborator

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

@bnoordhuis

bnoordhuis commented Mar 18, 2020

Copy link
Copy Markdown
Member

Before I review this: when or why (or to whom) would this be useful?

@cjihrig

cjihrig commented Mar 18, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Is this something that could leverage the existing NODE_DEBUG_NATIVE environment variable, instead of adding a new mode?

@gabrielschulhof

gabrielschulhof commented Mar 18, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@cjihrig I'd love to switch to NODE_DEBUG_NATIVE but, according to the documentation, its value is a list of native modules to debug, whereas the large pages statistics do not originate from any built-in native module, but rather from the runtime itself.

Is it within the semantics of NODE_DEBUG_NATIVE that I should could fprintf(stderr, ... if NODE_DEBUG_NATIVE is set to any non-NULL value?

@gabrielschulhof

gabrielschulhof commented Mar 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis I've used these fprintfs to see if I can maximize the code getting mapped to large pages. Another use might be if you see no benefit from running with --use-largepages=on. Then you might run with --use-largepages=verbose and see if the code that is hot for you is getting snipped off because it is at the beginning or end of the range we remap.

@cjihrig

cjihrig commented Mar 18, 2020

Copy link
Copy Markdown
Contributor

according to the documentation, its value is a list of native modules to debug, whereas the large pages statistics do not originate from any built-in native module, but rather from the runtime itself.

I think that definition might only be loosely enforced in the code. I'd personally be fine with a large pages category (and would prefer it over this approach). I guess we could see if anyone else objects though.

Is it within the semantics of NODE_DEBUG_NATIVE that I should fprintf(stderr, ... if NODE_DEBUG_NATIVE is set to any non-NULL value?

I don't think you'd use fprintf() explicitly. There are helpers in place already. This is how it's done in WASI, for example.

@gabrielschulhof

gabrielschulhof commented Mar 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@cjihrig thanks for the pointer!

@jasnell

jasnell commented Mar 18, 2020

Copy link
Copy Markdown
Member

I would definitely prefer just using NODE_DEBUG_NATIVE for this. You can easily add a new category specific for this and it doesn't have to be limited to just debug output for modules.

@gabrielschulhof

gabrielschulhof commented Mar 19, 2020

Copy link
Copy Markdown
ContributorAuthor

@jasnell@cjihrig I looked at the implementation of Debug, and it requires an Environment*. The problem is, when this code runs, the Environment* is not yet created. So, I would have to check NODE_DEBUG_NATIVE by hand.

@jasnell

jasnell commented Mar 19, 2020

Copy link
Copy Markdown
Member

hmm ok. Is it possible/reasonable to defer the logging until Environment is available?

@gabrielschulhof

gabrielschulhof commented Mar 19, 2020

Copy link
Copy Markdown
ContributorAuthor

@jasnell looks like there might be a way to use the debug without an Environment 🤔

Gabriel Schulhof
src: add debug option to report large page stats
436cc4d
This adds the new option `HUGEPAGES` to `NODE_DEBUG_NATIVE` that
causes the code responsible for re-mapping to large pages to output
memory range and page count information to `stderr`.
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
@gabrielschulhof

gabrielschulhof commented Mar 20, 2020

Copy link
Copy Markdown
ContributorAuthor

@jasnell@cjihrig I updated the PR to use Debug().

@nodejs-github-bot

nodejs-github-bot commented Mar 20, 2020

Copy link
Copy Markdown
Collaborator

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

@gabrielschulhof

gabrielschulhof commented Mar 20, 2020

Copy link
Copy Markdown
ContributorAuthor

Landed in c160073.

gabrielschulhof pushed a commit that referenced this pull request Mar 20, 2020
Gabriel Schulhof
src: add debug option to report large page stats
c160073
This adds the new option `HUGEPAGES` to `NODE_DEBUG_NATIVE` that
causes the code responsible for re-mapping to large pages to output
memory range and page count information to `stderr`.
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #32331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@gabrielschulhof
gabrielschulhof deleted the large-pages-verbose branch March 20, 2020 17:45
MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Gabriel Schulhof@MylesBorins
src: add debug option to report large page stats
243d0d4
This adds the new option `HUGEPAGES` to `NODE_DEBUG_NATIVE` that
causes the code responsible for re-mapping to large pages to output
memory range and page count information to `stderr`.
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #32331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 24, 2020
Merged
@targos

targos commented Apr 22, 2020

Copy link
Copy Markdown
Member

Depends on the large pages change to land on v12.x

targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
Gabriel Schulhof@targos
src: add debug option to report large page stats
193d634
This adds the new option `HUGEPAGES` to `NODE_DEBUG_NATIVE` that
causes the code responsible for re-mapping to large pages to output
memory range and page count information to `stderr`.
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs#32331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
targos pushed a commit that referenced this pull request Apr 28, 2020
Gabriel Schulhof@targos
src: add debug option to report large page stats
df046de
This adds the new option `HUGEPAGES` to `NODE_DEBUG_NATIVE` that
causes the code responsible for re-mapping to large pages to output
memory range and page count information to `stderr`.
Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #32331
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@cjihrigcjihrigcjihrig approved these changes
@lundibundilundibundiAwaiting requested review from lundibundi
+1 more reviewer
@devnexendevnexendevnexen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@gabrielschulhof@nodejs-github-bot@bnoordhuis@cjihrig@jasnell@targos@devnexen