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

[joyeecheung]

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-*)

[joyeecheung]

cc @nodejs/diagnostics

[targos]

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

[joyeecheung]

@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.

[mmarchini]

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.

[joyeecheung]

@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).

[joyeecheung]

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.

[vmarchaud]

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.

[joyeecheung]

@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)

[Flarna]

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

[vmarchaud]

@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.

[BridgeAR]

@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.