Make --heap-snapshot-on-oom use --diagnostic-dir

[Luzgan]

What is the problem this feature will solve?

I was looking into --heap-snapshot-on-oom parameter and right now it is not respecting the --diagnostic-dir parameter like --heapsnapshot-near-heap-limit. We have a setup which requires us to place dumps into a folder which is a s3 mount. And it would be much easier to be able to set the directory straight away, rather than copy afterwards.

What is the feature you are proposing to solve the problem?

Can we make so, that --heap-snapshot-on-oom will use diagnostic-dir as well?
While on that topic, would it be possible to change name of those heapdumps via parameter?

What alternatives have you considered?

Right now I am using --heapsnapshot-near-heap-limit=1 instead, but if possible I would prefer to use --heap-snapshot-on-oom, to make sure that only one snapshot will be generated (I am afraid of a case where files will be deleted and created again while using near heap limit 1)

[joyeecheung]

I am afraid of a case where files will be deleted and created again while using near heap limit 1

That is not going to happen, the limit only means that Node.js will stop generating more snapshots beyond this number.

--heap-snapshot-on-oom is not implemented by Node.js, I am not sure where it comes from?

[richardlau]

--heap-snapshot-on-oom is not implemented by Node.js, I am not sure where it comes from?

V8, according to #50711 (comment) 🙂

node/deps/v8/src/profiler/heap-profiler.h

Lines 64 to 65 in 6012a4e

	// Implementation of --heap-snapshot-on-oom.
	void WriteSnapshotToDiskAfterGC();

which is

node/deps/v8/src/profiler/heap-profiler.cc

Lines 169 to 188 in 6012a4e

	// Precondition: only call this if you have just completed a full GC cycle.
	void HeapProfiler::WriteSnapshotToDiskAfterGC() {
	// We need to set a stack marker for the stack walk performed by the
	// snapshot generator to work.
	heap()->stack().SetMarkerIfNeededAndCallback([this]() {
	int64_t time = V8::GetCurrentPlatform()->CurrentClockTimeMilliseconds();
	std::string filename = "v8-heap-" + std::to_string(time) + ".heapsnapshot";
	v8::HeapProfiler::HeapSnapshotOptions options;
	std::unique_ptr<HeapSnapshot> result(
	new HeapSnapshot(this, options.snapshot_mode, options.numerics_mode));
	HeapSnapshotGenerator generator(result.get(), options.control,
	options.global_object_name_resolver, heap(),
	options.stack_state);
	if (!generator.GenerateSnapshotAfterGC()) return;
	FileOutputStream stream(filename.c_str());
	HeapSnapshotJSONSerializer serializer(result.get());
	serializer.Serialize(&stream);
	PrintF("Wrote heap snapshot to %s.\n", filename.c_str());
	});
	}

[joyeecheung]

Oh yeah I forgot about my own comment 😂 I don't think there is much Node.js can do for affecting --heap-snapshot-on-oom, as V8 generally do not respect embedder flags. It would be more practical to either open a feature request in V8's issue tracker to add a separate flag that allows customization of --heap-snapshot-on-oom locations, or open a feature request in V8 to expose something like v8::internal::HeapProfiler::WriteSnapshotToDiskAfterGC() to the public API so that Node.js can re-implement it with the --diagnostic-dir it parses from CLI.

[luzgan-bpim]

That is not going to happen, the limit only means that Node.js will stop generating more snapshots beyond this number.

Oh, ok. Then I misunderstood how it works. I thought that this was the number of snapshots it will keep and then it will keep replacing. In this case - potentially - I am about to get close to the max heap, but won't OOM, it will create one snapshot and any future close to max won't do the job. I mean... technically this close to max heap can be treated as a problem for OOM verification as well, but it MAY come from different time than actual OOM. But maybe its nitpicking and in most cases it will give me what I need.

--heap-snapshot-on-oom is not implemented by Node.js, I am not sure where it comes from?

I see you got the answer, I wasn't aware that its actually coming from V8, my bad 😄 And totally agree, I would raise an issue there, although first I need to make sure that there wasn't anything in the documentation of V8 😄 (but based on the code, I dont think so)

This one can be closed 😄

[Luzgan]

Oh. I used wrong account, thats why I couldn't close. Dummy.