Expose cppgc::CppHeap::CollectStatistics() #57146
Conversation
nodejs-github-bot
added
c++
Aditi-1400
force-pushed
the
issue-56533
branch
2 times, most recently
from
0f5a16c to
dac6b7a
Compare
src/node_v8.cc
Outdated
|
|
||
| cppgc::HeapStatistics stats = isolate->GetCppHeap()->CollectStatistics( | ||
| static_cast<cppgc::HeapStatistics::DetailLevel>( | ||
| args[0].As<v8::Int32>()->Value())); |
There was a problem hiding this comment.
Nit: Add using v8::Int32; to the list of other using v8:... items at the top of the namespace and change this to just .As<Int32>()
There was a problem hiding this comment.
Side note... we do this kind of static cast so often we should probably just wrap it in a utility macro.... but no need to do that here
There was a problem hiding this comment.
(About that side note, maybe it's even better to wrap them in methods with templated return types for a bit of type checking, the integer types and enums can get especially hairy because there are so many of variants of them, I think FromV8Value<T>() would be very cool)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57146 +/- ##
==========================================
+ Coverage 89.10% 90.34% +1.23%
==========================================
Files 665 629 -36
Lines 193188 184551 -8637
Branches 37207 36041 -1166
==========================================
- Hits 172137 166724 -5413
+ Misses 13772 10957 -2815
+ Partials 7279 6870 -409
|
Aditi-1400
force-pushed
the
issue-56533
branch
from
dac6b7a to
b1e245b
Compare
lib/v8.js
Outdated
| }; | ||
|
|
||
| function getCppHeapStatistics(type = 'detailed') { | ||
| validateOneOf(type, 'type', ['brief', 'detailed', undefined]); |
There was a problem hiding this comment.
The undefined here is unnecessary since you are specifying the default value in the argument (it will never come through as undefined)
There was a problem hiding this comment.
| validateOneOf(type, 'type', ['brief', 'detailed', undefined]); | |
| validateOneOf(type, 'type', ['brief', 'detailed']); |
Expose `CppHeap` data via `cppgc::CppHeap::CollectStatistics()` in the v8 module.
Aditi-1400
force-pushed
the
issue-56533
branch
from
b1e245b to
bee4733
Compare
| FIXED_ONE_BYTE_STRING(isolate, "free_list_stats")}; | ||
|
|
||
| Local<Value> name_value; | ||
| if (!ToV8Value(context, stats.space_stats[i].name.c_str(), isolate) |
There was a problem hiding this comment.
| if (!ToV8Value(context, stats.space_stats[i].name.c_str(), isolate) | |
| if (!ToV8Value(context, stats.space_stats[i].name, isolate) |
.name is a std::string, so this should work fine here, right? It probably doesn't make a ton of difference in this method but if there's the option to stick to the strictly better (more performant/handles null bytes correctly) method, let's do it
| @@ -498,6 +699,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { | |||
| registry->Register(GCProfiler::New); | |||
| registry->Register(GCProfiler::Start); | |||
| registry->Register(GCProfiler::Stop); | |||
| registry->Register(getCppHeapStatistics); | |||
There was a problem hiding this comment.
Just a tiny style suggestion, but as you can see here, the convention is mostly that the C++ functions are named in UpperCamelCase
| * `detailLevel` (`'brief'`|`'detailed`): Specifies the level of detail in the | ||
| returned statistics. | ||
| * `brief`: Brief statistics contain only the top-level allocated and used | ||
| memory statistics for the entire heap. | ||
| * `detailed`: Detailed statistics also contain a break down per space and | ||
| page, as well as freelist statistics and object type histograms. |
There was a problem hiding this comment.
| * `detailLevel` (`'brief'`|`'detailed`): Specifies the level of detail in the | |
| returned statistics. | |
| * `brief`: Brief statistics contain only the top-level allocated and used | |
| memory statistics for the entire heap. | |
| * `detailed`: Detailed statistics also contain a break down per space and | |
| page, as well as freelist statistics and object type histograms. | |
| * `detailLevel` {string|undefined}: **Default:** `'detailed'`. Specifies the level of detail in the returned statistics. | |
| Accepted values are: | |
| * `'brief'`: Brief statistics contain only the top-level allocated and used | |
| memory statistics for the entire heap. | |
| * `'detailed'`: Detailed statistics also contain a break down per space and | |
| page, as well as freelist statistics and object type histograms. |
I am not exactly sure what's the best way to document these enums, but we do have some precedents of this style in the docs.
| @@ -271,6 +271,89 @@ buffers and external strings. | |||
| } | |||
| ``` | |||
|
|
|||
| ## `v8.getCppHeapStatistics(detailLevel)` | |||
There was a problem hiding this comment.
| ## `v8.getCppHeapStatistics(detailLevel)` | |
| ## `v8.getCppHeapStatistics([detailLevel])` |
We use the square brackets for optional arguments in the signature
| [`cppgc::HeapStatistics`][] object. You can learn more about the properties of the object in | ||
| the [V8 documentation][`cppgc::HeapStatistics struct`]. |
There was a problem hiding this comment.
| [`cppgc::HeapStatistics`][] object. You can learn more about the properties of the object in | |
| the [V8 documentation][`cppgc::HeapStatistics struct`]. | |
| [`cppgc::HeapStatistics`][] object. See the [V8 documentation][`cppgc::HeapStatistics struct`] | |
| for more information about the properties of the object. |
(I am fairly certain that we still try to avoid "you" as much as possible in the docs..)
Expose
CppHeapdata viacppgc::CppHeap::CollectStatistics()in the v8 module.Fixes: #56533