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

doc: move perf tools and APIs to Tier 3 #22915

Closed

Conversation

mmarchini
Copy link
Contributor

V8 CodeEventHandler, Linux perf and the --interpreted-frames-native-stack flag satisfies our Diagnostics Support requirements for Tier 3. V8 CodeEventHandler is tested as part of our V8 CI, and Linux perf / --interpreted-frames-native-stack are tested by test/v8-updates/test-linux-perf.js.

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

cc @nodejs/diagnostics

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 17, 2018
@mhdawson
Copy link
Member

I think we could leave the target tier in the tiers. For example if its in tier 3 it might still have a target or 1

@mmarchini
Copy link
Contributor Author

Update:

  • Include --prof, --prof-process and V8 CpuProfiler as discussed in Bump node-clinic to tier 3 diagnostics#234
  • Restore the Target Tier columns
  • Set Target Tier of V8 CodeEventHandler, --interpreted-frames-native-stack and Linux perf to 2.

V8 CodeEventHandler, V8 CpuProfiler, Linux perf and the
`--interpreted-frames-native-stack`, `--prof` and `--prof-process` flags
satisfies our Diagnostics Support requirements for Tier 3. V8
CodeEventHandler and CpuProfiler are tested as part of our V8 CI, and
`Linux perf` / `--interpreted-frames-native-stack` are tested by
`test/v8-updates/test-linux-perf.js`. `--prof` and `--prof-process` are
tested in Node.js test suite.
@mmarchini mmarchini force-pushed the linux-perf-diagnostics-support-tier branch from 8345456 to bb71034 Compare September 26, 2018 14:34
@mmarchini
Copy link
Contributor Author

| Tool Type | Tool/API Name | Regular Testing in Node.js CI | Integrated with Node.js | Target Tier |
|-----------|--------------------------------------|-------------------------------|-------------------------|-------------|
| Profiling | V8 CPU profiler | Partial (V8 Tests) | Yes | 1 |
| Profiling | --prof/--prof-process flags | Yes | Yes | 1 |
Copy link
Member

Choose a reason for hiding this comment

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

How come these flags are separate to the V8 CPU profiler? And the current tests do not properly test everything in depth, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How come these flags are separate to the V8 CPU profiler?

Because they are separate things: V8 CpuProfiler is a C++ API officially supported by V8 which can also be used through the inspector protocol. --prof is a flag not officially supported by V8.

And the current tests do not properly test everything in depth, right?

Right. That's why we're moving them to Tier 3 and not 2/1.

Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe say it's also partial in that case until we have more tests in place?

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.

It still feels strange to me that we put V8 stuff in our support tiers, but as long as they don't block our releases, LGTM

@mmarchini
Copy link
Contributor Author

IMO we should either support those things - preferably blocking, at least on LTS - or throw warnings to show users these features are unstable and can break at any time. Otherwise users might trust these things are stable and they might get stuck in a EOL version because they are relying heavily in some unsupported features.

@joyeecheung
Copy link
Member

joyeecheung commented Sep 26, 2018

Otherwise users might trust these things are stable and they might get stuck in a EOL version because they are relying heavily in some unsupported features.

I think this should only apply to LTS releases - it doesn't make too much sense to block a odd-number release with that reasoning, does it? I don't think it would get in our way if it only applies to LTS, since we try not to update V8 in LTS anyway.

@mmarchini
Copy link
Contributor Author

I think this should only apply to LTS releases - it doesn't make too much sense to block a odd-number release with that reasoning, does it? I don't think it would get in our way if it only applies to LTS, since we try not to update V8 in LTS anyway.

Yeah, I guess. Having these tools stable inside an LTS is the least we can do.

I'm still kinda worried though. All profiling and heap analysis solutions we have on Node.js today are actually V8 APIs and flags. Maybe core should expose stable APIs for these use cases, and we can keep them working even if V8 changes their APIs entirely (as suggested in nodejs/abi-stable-node#296)?

@Flarna
Copy link
Member

Flarna commented Sep 26, 2018

Another topic is test coverage and test quality. Not sure if V8 team spends the same energy into testing diagnostic tools as in the productive features - it would be valid to prioritize production features.

Besides that the use cases/requirements in node differ from browser in some aspects. A memleak or busy loop for diagnostic tools running in browser is most likely not that critical as in a server application as the impact is limited.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mmarchini
Copy link
Contributor Author

mmarchini commented Oct 1, 2018

@mmarchini
Copy link
Contributor Author

Landed in fa14503

@mmarchini mmarchini closed this Oct 1, 2018
mmarchini added a commit that referenced this pull request Oct 1, 2018
V8 CodeEventHandler, V8 CpuProfiler, Linux perf and the
`--interpreted-frames-native-stack`, `--prof` and `--prof-process` flags
satisfies our Diagnostics Support requirements for Tier 3. V8
CodeEventHandler and CpuProfiler are tested as part of our V8 CI, and
`Linux perf` / `--interpreted-frames-native-stack` are tested by
`test/v8-updates/test-linux-perf.js`. `--prof` and `--prof-process` are
tested in Node.js test suite.

PR-URL: #22915
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Oct 3, 2018
V8 CodeEventHandler, V8 CpuProfiler, Linux perf and the
`--interpreted-frames-native-stack`, `--prof` and `--prof-process` flags
satisfies our Diagnostics Support requirements for Tier 3. V8
CodeEventHandler and CpuProfiler are tested as part of our V8 CI, and
`Linux perf` / `--interpreted-frames-native-stack` are tested by
`test/v8-updates/test-linux-perf.js`. `--prof` and `--prof-process` are
tested in Node.js test suite.

PR-URL: #22915
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants