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: V8 Sampling Heap Profiler current state #259

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

Tier of Support: V8 Sampling Heap Profiler current state #259

Drieger opened this issue Dec 5, 2018 · 6 comments

Comments

@Drieger
Copy link

Drieger commented Dec 5, 2018

I tried to summarize current state of requisites for V8 Sampling Heap Profiler. 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 V8 Sampling Heap Profiler
URL https://github.com/v8/sampling-heap-profiler
Type Memory
Target Tier 1
Requisite Status Observation
Must always be working (CI tests passing) for all LTS Node.js Ok Tests are passing and runs on every LTS Node.js
Have a good test suite Maybe Test suite exists. Not sure if it is complete yet, based on the following open issues
https://github.com/v8/sampling-heap-profiler/issues/2
https://github.com/v8/sampling-heap-profiler/issues/14
Test suite job must exist in Node.js CI Fail Job do not exist in Node.js CI
The maintainers of the tool must remain responsive when there are problems Maybe The tool has a few maintainers. Today mainly active maintainer seems to be @ofrobots
The tool must be actively used by the ecosystem Maybe The tool does not seem to be used by a large part of the ecosystem.
[npm] 253 weekly downloads
The tool must be heavily depended on False Diagnostic tools are never depended on to run systems. Although it should be interesting to make sure that these tools are never broken, so they can be used when necessary. We could try to use npm dependents, but it doesnt't seems to be a good approach
The tool must have a guide or other documentation in the Node.js GitHub organization or website Fail No guide or documentation found in the Node.js GitHub organization or website. Project has only a README file in its own repository with a some usage explanation
The tool must be working on all supported platforms Ok Works on all platforms where V8 heap profiler is also working
The tool must only be using APIs exposed by Nodejs as opposed to its dependencies Ok The tool seems to be using only exposed APIs
The tool must be open source Ok The tool is open source under Apache License 2.0 https://github.com/v8/sampling-heap-profiler/blob/master/LICENSE

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 V8 Sampling Heap Profiler to target tier. It is also an opportunity to debate if requirements are correct. Any ideas and opinions are welcome.

@mcollina
Copy link
Member

mcollina commented Dec 5, 2018

I think most of the activities are on the Node.js side.
I'm +1 on moving it to Tier 1, however I'm a bit skeptical to because of the big "Experimental" warning at the top.
I don't think Tier 1 tools should be experimental.

@ofrobots
Copy link
Contributor

ofrobots commented Dec 5, 2018

Thanks for starting this thread @Drieger. One thing to clarify would be the distinction between the V8 Sampling Heap Profiler API and the user-space module (that I maintain) at https://github.com/v8/sampling-heap-profiler. At first, I think we take the API itself through the rigor to figure out the tier of support. Whether or not my user-space implementation is the best way to consume the V8 API may be a different topic.

An alternative user-space implementation could become better than mine, or alternatively we could argue that this is important enough to bake directly into Node.js itself.

BTW, note that @google-cloud/profiler uses the code from my module. It has ~80k weekly downloads. The plan is for this module to start directly depending on heap-profile once we have pre-built binaries built and published

P.S. It would be great to have more collaborators on heap-profile. If anyone is interested, let me know.

@joyeecheung
Copy link
Member

I think this is worth being added to the v8 module, along with a JS API to trigger heapdumps (there was a feature request, couldn’t find it ATM)

@vmarchaud
Copy link
Contributor

I believe you are referring to nodejs/node#23328.

I will point out the same point as i did for having a API in core for taking heapdump : We already have this API through the inspector API, wouldn't be an overhead to maintain both ?
Or maybe that the solution of having a high level API built on top of inspector make sense, maybe in a userland package ? We could also implement the specific case like filtering frame from the inspector itself for profilers (CPU/Heap)

@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

6 participants