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

Tier of Support: Linux perf and V8 --interpreted-frames-native-stack current state #261

Closed
Drieger opened this issue Dec 10, 2018 · 6 comments

Comments

@Drieger
Copy link

Drieger commented Dec 10, 2018

I tried to summarize current state of requisites for Linux perf. Requisites are taken from Tier 1 (most complete set of requirements).

There are some requirements that seem to be subjective (at least to me), which means it's not clear if a tool fulfills it or not.

Requisites

  • Must always be working (CI tests passing) for all LTS Node.js
  • Have a good test suite
  • Test suite job must exist in Node.js CI
  • The maintainers of the tool must remain responsive when there are problems (Subjective)
  • The tool must be actively used by the ecosystem (Subjective)
  • The tool must be heavily depended on (Subjective)
  • The tool must have a guide or other documentation in the Node.js GitHub organization or website
  • The tool must be working on all supported platforms
  • The tool must only be using APIs exposed by Nodejs as opposed to its dependencies
  • The tool must be open source.

Tool Analysis

Name Linux perf
URL https://github.com/torvalds/linux/tree/master/tools/perf
Type Profiling
Current Tier 3 (according to guide)
Target Tier 2
Requisite Status Observation
Must always be working (CI tests passing) for all LTS Node.js Maybe It isn't related specific to Node.js versions. And --perf-basic-prof is working correctly
Have a good test suite Ok Tool has a good test suite
Test suite job must exist in Node.js CI Ok Test might be found on test/v8-updates/test-linux-perf.js
The maintainers of the tool must remain responsive when there are problems Ok As this tool is under Linux development community, maintainers should be fairly responsive. Also --perf-basic-prof is under V8 supervision
The tool must be actively used by the ecosystem Maybe Hard to tell as this is an analysis tool that comes with many Linux distributions
The tool must be heavily depended on Fail Diagnostic/analysis tools are almost never depended on to run systems.
The tool must have a guide or other documentation in the Node.js GitHub organization or website Fail Tool doesn't have any guide in the Node.js Github organization or website. Tool has a good documentaion https://github.com/torvalds/linux/tree/master/tools/perf/Documentation
--perf-basic-prof has almost no documentation in organization Github or website
The tool must be working on all supported platforms Fail Tool only works on Linux based platforms
The tool must only be using APIs exposed by Nodejs as opposed to its dependencies Ok Tools has no dependencies from Node.js. Only dependency we might consider here is to use the `--perf-basic-prof` or related flags
The tool must be open source Ok Tool is open source, under the GNU GPL. --perf-basic-prof is under V8 and is open source as well

Based on this it is possible to have a more clear view of the current state and what is necessary to work on to move Linux perf to target tier. It is also an opportunity to debate if requirements are correct. Any ideas and opinions are welcome.

@mcollina
Copy link
Member

I think we should move The tool must be working on all supported platforms to the "if-applicable" category, and relax this for Linux perf.

Diagnostic/analysis tools are almost never depended on to run systems.

This will be true for all tools that are tracked via this process. IMHO perf is lightly depended on, as several companies are depending on this to work, e.g. Netflix. We used it quite ofter at NearForm as well.

Must always be working (CI tests passing) for all LTS Node.js

I think perf integration is broken on Node 8, and it's not really fixable. @mmarchini added a flag to fix the data in Node 10+.

@naugtur
Copy link
Contributor

naugtur commented Dec 15, 2018

As for the docs/guide

I've been working on a guide for flame graphs using perf and it's been almost finished for a while, but I couldn't get it to display correct function names (yes, with the new flags) because of OS support issues. I wanted to figure those out before finally posting.
I'm hoping to finish it soon

nodejs/nodejs.org#1444

@Drieger
Copy link
Author

Drieger commented Dec 17, 2018

Great @naugtur 😄
As soon as it is merged we can update the state here.

@Drieger
Copy link
Author

Drieger commented Dec 17, 2018

As pointed by @mcollina in #262, it makes more sense to evaluate perf and --interpreted-frames-native-stack together, so I will move --interpreted-frames-native-stack current state to continue its evaluation here.

Tool Analysis

Name V8 --interpreted-frames-native-stack
Type Profiling
Current Tier 3 (according to guide)
Target Tier 2
Requisite Status Observation
Must always be working (CI tests passing) for all LTS Node.js Maybe Flag was added on Node.js v10.x. So since that version, all LTS Node.js should be ok
Have a good test suite Maybe Test exists for the feature. I'm not sure if test coverage enough
Test suite job must exist in Node.js CI Maybe Tests are run based on V8 (test/v8-updates/test-linux-perf.js)
The maintainers of the tool must remain responsive when there are problems Ok This is mantained as part of V8, so should be ok
The tool must be actively used by the ecosystem Maybe No metrics available to define this
The tool must be heavily depended on Maybe It is hard to define it, specially if we think that the ecosystem could be only people interested in diagnostics/profiling/...
The tool must have a guide or other documentation in the Node.js GitHub organization or website Fail Tool doesn't have any guide in the Node.js Github organization or website.README file in its own repository with a some usage explanation
The tool must be working on all supported platforms True Works on all platforms where V8 is also working
The tool must only be using APIs exposed by Nodejs as opposed to its dependencies Fail Tool must use some of its dependencies. But it is necessary for this kind of tool
The tool must be open source Ok Tool is part of V8

@Drieger Drieger changed the title Tier of Support: Linux perf current state Tier of Support: Linux perf and V8 --interpreted-frames-native-stack current state and Dec 17, 2018
@Drieger Drieger changed the title Tier of Support: Linux perf and V8 --interpreted-frames-native-stack current state and Tier of Support: Linux perf and V8 --interpreted-frames-native-stack current state Dec 17, 2018
@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

@github-actions
Copy link

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants