RFC: add a flag --heap-prof (similar to --cpu-prof but for the heap)

Refs: #26878

I am working on a flag --heap-prof here similar to --cpu-prof and would like to see the general feeling about this flag. It's pretty similar to --cpu-prof, and reuses most of the CPU profiler and coverage integration (with an abstract class).

$ ./node --heap-prof -p process.versions
$ ls *.heapprofile
Heap.20190426.140439.43866.0.001.heapprofile

When opening the profile in the Chrome DevTools it looks like this:

(to my surprise makeNodeErrorWithCode allocates quite an amount of memory during startup)

There is still a remaining issue with the current implementation (and it's also the case for --cpu-prof but reproduce less often): the default sampling interval is too high which makes the test quite flaky - but we can't allocate too much memory in the test in case it times out in the CI on the less powerful machines. It would be pretty simple to add --cpu-prof-interval/--heap-prof-interval and make them customizable, but I am not sure if people are comfortable with this kind of API design (passing everything with flags, though that's already the case for --report-*)

cc @nodejs/diagnostics

is it possible to combine both flags and get two profiles in one run?

@targos

@targos In terms of implementation, it is possible. Maybe we could just add a flag --prof-all or something that implies both --cpu-prof and --heap-prof, then it's just an option parser alias thing which is trivial to implement.

I like the idea, having a flag makes it easier for development and lowers the barrier for users.

But on production it would be more useful to have an API that allows us to start and get the heap profile during runtime. This would allow to take the profile at different points in time and compare them to see what changed (which can help users to identify which functions are misbehaving). Also, getting the profile without saving to disk can be useful for users of serverless or users who send the profile to another place and want to avoid the unnecessary overhead of writing to disk.

Having both the flag and the API would be ideal in my opinion.

@mmarchini

@mmarchini Yes, these profiling functionalities would be more useful for servers if they can be turned on/off on demand - in terms of programmable API there the inspector module already allows you to do these, although it is less straightforward to use.

Another thing that we need to consider for production-use is the serializer implementation. As mentioned in #26878 I think it is fine to use the inspector from C++ and do JSON::Parse and then JSON::Stirngify for these CLI flags as a start, as it saves us the trouble of maintaining the serializer, and it probably does not really matter in the case of local tests. For profiling in production on demand, this matters more (as your process would continue to run after the profile serialization).

Implementation-wise, for profiling on-demand I think the ideal solution is to expose JSON serializers for the profiles in the V8 embedder API (like HeapSnapshot::Serialize in v8-profiler.h) and make sure they are tested in the upstream, so we could transfer the maintenance cost to v8, provided that they are okay with this (cc @nodejs/v8-inspector) - it is similar in the spirit of https://chromium-review.googlesource.com/c/v8/v8/+/1546559 in that we need something exposed in the V8 embedder API instead of being encapsulated in the inspector implementation, or otherwise we need to jump through a lot more hoops to implement a functionality (which matters for long-running servers in production).

I think one of the things we should get to before moving these flags out of experiment is a consistent API to trigger these diagnostics functionalities (both --cpu-prof, --heap-prof, --report and possibly --heapsnapshot). One of the common trigger is signal (e.g. SIGUSR2) and it would be convenient to have shortcuts for those in the CLI as well (like --heapsnapshot-signal).

I opened https://bugs.chromium.org/p/v8/issues/detail?id=9182 in the upstream to see if the idea about having CpuProfile::Serialize() and AllocationProfile::Serialize() is acceptable.

Funny thing, i recently started to work on a piece of software that focus on profiling at runtime (specially in production).
I personally found out that the inspector module is a good enough API for that but a higher level implementation for users should be available in userland.

I've come up with a API that pretty much look like this:

const profilingAgent = new ProfilingAgent()
const session = new inspector.Session()
profilingAgent.register(new SignalTrigger({ signal: 'SIGUSR2' }), new InspectorHeapProfiler({ session }))
profilingAgent.register(new SignalTrigger({ signal: 'SIGUSR1' }), new InspectorCPUProfiler({ session }))
profilingAgent.start({ exporter: new FileExporter() })

I can't speak about the C++ implementation behind the inspector but i thought that since the topic of runtime profiling was brought up, i could get some feedback about how to make it available to more users.

@vmarchaud

@vmarchaud Thanks for the demo!

Is SignalTrigger here supposed to be a subclass of a more generic Trigger? And FileExporter supposed to be a subclass of a more generic Exporter?

I think if possible we should unify this with the process.report() API (luckily it's still in experiment)

Regarding use of inspector API in production: Are there any docs available regarding side-effects/overhead by just enabling inspector and create a session?

@joyeecheung

@joyeecheung Indeed, every profiler/trigger/exporter is an interface that can be implemented in his own package. If you want i just made public the project there.
I finished a first version of the core interfaces/concept and i'm working on tests for those, after that i will be implementing more profiler (trace events and the report api) and exporter (S3 would be a must have i believe)

If anyone has some feedback on it, it would be much appreciated.

@joyeecheung

@joyeecheung

to my surprise makeNodeErrorWithCode allocates quite an amount of memory during startup

I guess the reason for that is that we create ~210 Error classes in there.

src: refactor profile initialization

src: refactor V8ProfilerConnection to be more reusable

I think that the flag are easier to understand for end user, in which case they don't need to understand the way inspector works nor add any code to their application.
From my understanding the inspector module is pretty much a tool given to APM vendor or node specialist to implement other tools on top of it.

Flag is easier - I agree but Imagine you have an npm module that can capture heap snapshot using inspector and instead of running node --profiler-flag index.js, you can run <this nice module cli> index.js instead and it actually hides inspector usage from you. This module should be pretty straightforward and I am going to implement it in next couple days. At the same time this module can be much more flexible and for example dump output in some different formats or analyze it using some heuristic, e.t.c.

I agree that it's easy to implement it userland when you already know the inspector API (i'm also working on an userland module), but there are two problems:

an userland module will be loaded after the nodejs bootstrap which already hide some allocation.
The main problem of an userland module would be a breaking changes on V8 that land since no test are there to validate the output on Node side. With a core API there, it's properly tested within the NodeJS CI.

We also could test the inspector API (or putting the module in CITGM) but i don't think it's preferable in the long run to have dedicated flags for debugging/profiling so they are better understood by end developers.

I agree that it's easy to implement it userland when you already know the inspector API (i'm also working on an userland module), but there are two problems:

I am not talking about using inspector bindings and load special script to the app. I am proposing to use separate node process that connects to inspected process using WebSocket, put nothing to inspected process user land and when using in combination with --inspect-brk has guarantee that it does not skip anything in user land.

All protocol methods are tested on V8 CI, protocol maintainers give you a guarantee that protocol API is stable. So I believe that inspector protocol is even more safe option from compatibility point of view than node flag.

I am not sure that in long run there is any difference in two mindsets:

I'm using --node-flag to get profile,
I'm using globally installed node CLI app to get profile.

Second one actually allows more people to work on making CLI tool better.

I built some proof of concept and release it on npm, please take a look: thetool

It is important that it uses out of process inspection over websocket instead of inspector bindings. With this approach we can avoid polluting inspected process with scripts required for inspector and fs.

I believe there are several constraints that come with this approach:

The user would need to have the permission to open a port for the WebSocket server and listen for incoming connections, that may create difficulties in certain environments like docker containers, depending on what the developers are allowed to do with them if they do not have control over their environment. AFAIK, we can't limit the inspector server to whitelist commands, and that may be a concern for those environments as well.
This requires the inspector protocol implementation, which means e.g. if you don't have crypto, you can't profile. With the current API surface, we can eventually replace the implementation with the embedder API (see discussions in inspector: implement --heap-prof #27596 (comment) ) and we could easily reuse the implementation to create runtime APIs for on-demand profiling.

And about making this functionality in the user land as an npm module v.s. being in core: while we used to leave this kind of stuff in the user land, we now tend to move them into core - recent examples are node-report (which was an npm module by itself) and --heapsnapshot-signal/v8.writeHeapSnapshot. There are several reasons for this:

User land modules tend to go unmaintained if there is not a dedicated team behind it. The core has a bigger contributor base. Yes, sometimes we have maintenance issues in certain subsystems where contributors with expertise are not available and reviews can be hard to request, but it's usually better than npm modules whose contributors are in a similar position, as there will be more people paying attention and contributing to the code even they are not the people who know it the best, and there is always a team of active administrators who can onboard new contributors and merge PRs responsibly even if they cannot maintain the subsystem themselves.
More people would get to know this functionality and use it if this is part of the core. There have been requests about exposing heap snapshot generation in core even though it can be done via the inspector and with npm modules all along - that is why we implemented --heapsnapshot-signal/v8.writeHeapSnapshot. Sometimes people would rather not use something if they have to install third-party code for it, or they would be hesitant about using what they could find in the npm search results - we do not even have a curated list of well-maintained packages for diagnostics so it's possible that people cannot find them at all if they don't know the correct keyword.

@eugeneo

The user would need to have the permission to open a port for the WebSocket server and listen for incoming connections, that may create difficulties in certain environments like docker containers, depending on what the developers are allowed to do with them if they do not have control over their environment. AFAIK, we can't limit the inspector server to whitelist commands, and that may be a concern for those environments as well.

It is good point, I would like to propose --inspect-pipe flag. Chromium can expose protocol using WebSocket or pipe right now for environments where WebSocket for some reasons does not work.
When this flag is passed then Node will expose protocol using pipe instead of WebSocket. I will speak with @eugeneo about it.

This requires the inspector protocol implementation, which means e.g. if you don't have crypto, you can't profile. With the current API surface, we can eventually replace the implementation with the embedder API (see discussions in #27596 (comment) ) and we could easily reuse the implementation to create runtime APIs for on-demand profiling.

I am not sure about this point. Why do we need crypto for inspector protocol implementation? Protocol implementation is self contained and located inside V8. If crypto is required for inspector web socket server implementation inside Node then using pipe instead of WebSocket will resolve this issue as well.

On-demand profiling can easily be done with out of process inspection as well. I will add something like this to thetool this evening. At the same time thetool works just fine with Node 10 and most likely most features work with Node 8 as well.

And about making this functionality in the user land as an npm module v.s. being in core: while we used to leave this kind of stuff in the user land, we now tend to move them into core - recent examples are node-report

It is definitely valid point as well. Could we then put thetool to deps, the same way as we have node-inspect, npx there? I believe that I can make it 0 dependencies package.

In general I am worried that we are building second API surface for the data that is already available when V8 team already has a commitment to maintain inspector protocol API and it has huge user base - Chrome DevTools, VSCode, puppeteer, e.t.c. - all of them use protocol. As a result we might get to the point when data exposed using flags from Node is different with data exposed using protocol from Chromium and Node and I can not build some nice tool to analyze V8 behavior that works equally great for V8 inside Chromium and for V8 inside Node anymore.

In any case, if we can add flags for profiling, could I add --inspect-pipe and --inspect-store flags. It will allow me to continue working on better protocol based tooling story and it would be possible later to compare adoption of protocol based solutions and flag based.

I am not sure about this point. Why do we need crypto for inspector protocol implementation?

I don't really know why, but this is currently the case if you build with ./configure --without-ssl (the help text says build without SSL (disables crypto, https, inspector, etc.), I think the inspector protocol requires WebSocket with SSL? (or otherwise it would be unsafe). I am not sure if switching to pipe resolves that constraint.

Could we then put thetool to deps, the same way as we have node-inspect, npx there?

It would be possible but I think we need to be sure that we actually want to stick with the inspector approach before doing that...is the inspector protocol suitable to use in production servers in general? Running an inspector server in your applications enables remote execution via Runtime methods, which is not really acceptable in production deployments that I know of, is there currently a way to put any policies or constraints on that? (But in general, turning inspector on in production sounds a bit scary to me)

In any case, if we can add flags for profiling, could I add --inspect-pipe and --inspect-store flags. It will allow me to continue working on better protocol based tooling story and it would be possible later to compare adoption of protocol based solutions and flag based.

I personally think --inspect-pipe would be great to have. The idea behind --inspect-store is more complicated so I'll defer that to nodejs/diagnostics#298

@eugeneo

I don't really know why, but this is currently the case if you build with ./configure --without-ssl (the help text says build without SSL (disables crypto, https, inspector, etc.), I think the inspector protocol requires WebSocket with SSL? (or otherwise it would be unsafe). I am not sure if switching to pipe resolves that constraint.

It looks like inspector source files use SHA1 from openssl. @eugeneo do you think that we can not make inspector work without ssl? We use ws:// scheme for inspector WebSocket so I assume that our connection is not secure. At the same time over the pipe we can use much simpler format - the same as Chromium use - pass messages as is with \0 as delimiter so pipe definitely does not require ssl.

It would be possible but I think we need to be sure that we actually want to stick with the inspector approach before doing that...is the inspector protocol suitable to use in production servers in general? Running an inspector server in your applications enables remote execution via Runtime methods, which is not really acceptable in production deployments that I know of, is there currently a way to put any policies or constraints on that? (But in general, turning inspector on in production sounds a bit scary to me)

I believe that inspector is suitable to use in production. It produces ~0 overhead if there is no active inspector session. Overhead of active session depends on what features are used, e.g. sampling heap profiler has very small overhead, enabled debugger sometimes might have huge overhead.
As long as inspector WebSocket is not available from outside which sound to me like regular normal configuration of any server, it should be safe to use (in any case it should be pretty straightforward to setup a server in a way when it does not expose inspector WebSocket to outside world). With pipe it would be even safer since pipe can only be accessed from local processes. So from security point of view and runtime overhead point of view, everything looks production ready.

In general it is not about policy to allow some domains and disable another domains but about making inspector channel (websocket or pipe) available for non local process or not available.

is the inspector protocol suitable to use in production servers in general?

Inspector infrastructure was designed to be used in production:

It is powering long-running stress testing pipelines in Chrome production, powers Telemetry performance tests in Chrome land.
Node-running cloud providers adopted it for their diagnostics pipelines already.
In the consumer field it became the single interface powering all the diagnostic tools: DevTools, Lighthouse, Puppeteer, WebDriver, VSCode and thousands other clients that are using it.

We know that it has zero performance overhead and withheld the test of load and evolution. I would claim that there is no other API that would stand close to these metrics in production. So if we want to see further adoption and the blooming of the tooling around diagnostics in Node, I would go all in.

Running an inspector server in your applications enables remote execution via Runtime methods, which is not really acceptable in production deployments that I know of

Diagnostics API should only be exposed to the trusted party. When running over the Web Socket It is not available to the one accessing an open port, it requires a crypto-secure unguessable token to be present in order to engage. When running as a pipe, it trusts the launcher process to handle this diagnostics channel in a secure manner.

@ak239

@ak239 @pavelfeldman Thanks for sharing your insights here!

I ran a Twitter poll to see if there are concerns from experiences using inspector protocol in the wild: https://twitter.com/JoyeeCheung/status/1128314669209272321 Most concerns were around the Debugger domain being accidentally enabled by the inspector client, though they seemed to be resolvable with upstream changes in the protocol.

However coming back to the original proposal - adding a --heap-prof flag to Node.js core, the discussions around using inspector server are more related to the implementation of the flag, not the use case of the feature itself. If we decide to integrate a dependency like thetool to initiate these operations in the process, I think it would be fairly straightforward to reimplement these functionalities without breaking changes. The flag is mostly a convenient shortcut, just like NODE_V8_COVERAGE, --cpu-prof and --heapsnapshot-signal. Since we already have these precedents and there are positive signals from the user land, I believe we could move forward with the addition of the --heap-prof flag, and leave the implementation details to be revisited in the future.

inspector: implement --heap-prof

Navigation Menu

Repository navigation

RFC: add a flag --heap-prof (similar to --cpu-prof but for the heap) #27421

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

RFC: add a flag --heap-prof (similar to --cpu-prof but for the heap) #27421

Description

Activity

joyeecheung commented on Apr 26, 2019

targos commented on Apr 26, 2019

joyeecheung commented on Apr 26, 2019

mmarchini commented on Apr 26, 2019

joyeecheung commented on Apr 26, 2019

joyeecheung commented on Apr 26, 2019

vmarchaud commented on Apr 26, 2019

joyeecheung commented on Apr 26, 2019

Flarna commented on Apr 26, 2019

vmarchaud commented on Apr 26, 2019

BridgeAR commented on Apr 27, 2019

4 remaining items

alexkozy commented on May 10, 2019

vmarchaud commented on May 10, 2019

alexkozy commented on May 10, 2019

alexkozy commented on May 11, 2019

joyeecheung commented on May 11, 2019

joyeecheung commented on May 11, 2019

alexkozy commented on May 11, 2019

alexkozy commented on May 11, 2019

joyeecheung commented on May 11, 2019

alexkozy commented on May 12, 2019

pavelfeldman commented on May 12, 2019

joyeecheung commented on May 20, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions