v8: avoid truncating cppgc heap statistics - #64696
Conversation
nodejs-github-bot
added
c++
Convert cppgc size_t counters directly to JavaScript Numbers instead of narrowing them to uint32_t. This prevents v8.getCppHeapStatistics() values from wrapping when a cppgc heap statistic exceeds 4 GiB. Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64696 +/- ##
==========================================
- Coverage 90.14% 90.13% -0.01%
==========================================
Files 741 741
Lines 242112 242107 -5
Branches 45605 45604 -1
==========================================
- Hits 218244 218224 -20
- Misses 15360 15379 +19
+ Partials 8508 8504 -4
🚀 New features to boost your workflow:
|
| Number::New(isolate, | ||
| static_cast<double>(object_stats.allocated_bytes)), | ||
| Number::New(isolate, | ||
| static_cast<double>(object_stats.object_count))}; |
There was a problem hiding this comment.
I wonder if we should instead optionally be handling these as bigint/uint64_t. Accept the truncation by default, add an option to return bigint instead...
v8.getCppHeapStatistics({ detailLevel: '...', bigint: true });There was a problem hiding this comment.
I wonder if we should instead optionally be handling these as bigint/uint64_t. Accept the truncation by default, add an option to return bigint instead...
v8.getCppHeapStatistics({ detailLevel: '...', bigint: true });
Could you explain a bit more ?I don't completely understand that why we should accept the data that wrap?
There was a problem hiding this comment.
I wonder if we should instead optionally be handling these as bigint/uint64_t. Accept the truncation by default, add an option to return bigint instead...
Are any of these realistically in danger of hitting kMaxSafeInteger?
|
@joyeecheung Please take look at this |
Renegade334
left a comment
Renegade334
left a comment
There was a problem hiding this comment.
This seems like a pure improvement to me, the likelihood of any of these values being exabytes in size seems exceedingly remote, whereas wrapping after a few gigabytes seems like a genuine pitfall.
4 participants
Convert cppgc size_t counters directly to JavaScript Numbers instead of narrowing them to uint32_t.
This prevents v8.getCppHeapStatistics() values from wrapping when a cppgc heap statistic exceeds 4 GiB.