feat(core): Deno.core.heapStats()

[AaronO]

This implements Deno.core.heapStats() so we can programatically measure isolate heap-usage (for benchmarks, etc...)

Sample output

❯ ./target/release/deno eval "console.log(Deno.core.heapStats())"

{
  total_heap_size: 3473408,
  total_heap_size_executable: 262144,
  total_physical_size: 2650012,
  total_available_size: 1515724108,
  total_global_handles_size: 8192,
  used_global_handles_size: 192,
  used_heap_size: 2874528,
  heap_size_limit: 1518338048,
  malloced_memory: 73860,
  external_memory: 5763,
  peak_malloced_memory: 58200,
  number_of_native_contexts: 1,
  number_of_detached_contexts: 0
}

Heap usage

.used_heap_size is the most relevant individual metric

With 1.8.0 it has grown from ~2MB to ~2.25MB (most likely due to the added JS for WebGPU, ~190kb of JS src)
I had originally reported 2MB => 3MB since I was sampling on a debug release

This is measured by:

❯ ./target/release/deno eval --v8-flags=--expose-gc "gc(); console.log(Deno.core.heapStats().used_heap_size/1e6)"
2.27596

Notes

• This is implemented as a core function since ops don't have explicit access to the isolate.
• It might make sense to integrate this with Deno.metrics at some point (to have a single func to get general metrics).
• I've preserved the naming of the props in snake case, can change that to camel case if required.
• Debug builds have a higher baseline than release (originally causing me to over-report the 1.8.0 increase)
• Baseline should be measured after a GC

[bnoordhuis]

Needs tests but other than that the technical side of the PR looks good to me, thanks.

Another maintainer should weigh in on the API.

[bnoordhuis]

I assume you're wrapping v8::HeapStatistics because that makes it easier to serialize in the future?

[AaronO]

Exactly, with serde_v8 (and the new op-ABI) all the set_prop() calls would become a one-liner to_v8() and by keeping it in a struct we can have serde handle the camelCase-ification (if desired).

There are other core binding funcs that will be simplified with serde_v8, e.g: get_proxy_details and get_promise_details conceptually return tuples, which serde_v8 can serialize to JS arrays (removing a fair amount of boilerplate in each function)

[AaronO]

I could probably add some simple alloc-dealloc tests (of large arrays) to ensure they are correctly reflected in the metrics.

Anything else that warrants testing in your opinion ?

[bartlomieju]

@AaronO such test would be sufficient

[caspervonb]

Nit; we should probably camelCase these properties?

[bartlomieju]

Nit; we should probably camelCase these properties?

Yes

[AaronO]

@caspervonb @bartlomieju Here's my reasoning:

• If it's exposed via Deno.core.* I would simply follow the internal naming conventions leave it in snake_case
• If it's exposed for "broad-consumption" in Deno.* I would not only switch it to camelCase but also rename the keys, since they're currently quite verbose (given that if they're behind a heapStats() call or a .heap sub-obj of Deno.metrics() context clarifies meaning)

Maybe something like this:

{
  heapTotal,
  heapExecutable,
  physical,
  available,
  globalHandlesTotal,
  globalHandlesUsed,
  heapUsed,
  heapLimit,
  malloced,
  external,
  mallocedPeak,
  nativeContexts,
  detachedContexts,
}

(Not alphabetically sorted/grouped with new names)

vs

{
  total_heap_size,
  total_heap_size_executable,
  total_physical_size,
  total_available_size,
  total_global_handles_size,
  used_global_handles_size,
  used_heap_size,
  heap_size_limit,
  malloced_memory,
  external_memory,
  peak_malloced_memory,
  number_of_native_contexts,
  number_of_detached_contexts,
}

[ry]

Looks good to me, but this should wait for 1.9.0, I think. Unfortunately that's 6 weeks out as we just released 1.8.0 yesterday.

It could be argued that Deno.core is not part of the public API. It's not listed in "deno types" so maybe this could go out in a patch release. Interested in other's opinions...

[lucacasonato]

I think we should just land for 1.9. Merge window opens on 17th right? It will be in main for 4 weeks then. As this will mainly be used for benchmarking, it only needs to be in main, not necessarily in a release right away.

[bartlomieju]

It could be argued that Deno.core is not part of the public API. It's not listed in "deno types" so maybe this could go out in a patch release. Interested in other's opinions...

I agree with that, we've already did this several times, I see no point in holding it off.

That said I don't like that the fields are using snake_case instead of camelCase.

[AaronO]

@bartlomieju I'll switch this to camelCase and add tests later today, so we can merge this then (I was hoping to camelCase through serde with #9722, but we can do that later)

I'll do a separate PR on adding this "baseline isolate heap" to the benchmarks since there's some meta-discussion to be had on where to put it (doesn't perfectly fit with the other "max memory" benches and also differs from them by an order of magnitude)

[AaronO]

@bartlomieju The windows test release failed for an unrelated (DNS error), I don't think I can retry it, could you ?

Otherwise linting & tests pass.

[bartlomieju]

@AaronO done, I think this is good to land as is

[AaronO]

@bartlomieju great, thanks !