Skip to content

src,lib: support path in v8.writeHeapSnapshot output #58959

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juanarbol
Copy link
Member


I'm a bit rusty with my C++. Any feedback will be more than valuable.

@juanarbol juanarbol requested a review from BridgeAR July 5, 2025 06:32
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 5, 2025
@juanarbol juanarbol changed the title Juan/filepath v8 src,lib: support path in v8.writeHeapSnapshot output Jul 5, 2025
Copy link

codecov bot commented Jul 5, 2025

Codecov Report

Attention: Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.02%. Comparing base (cc856b3) to head (0055948).
Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
src/heap_utils.cc 72.00% 3 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58959      +/-   ##
==========================================
- Coverage   90.09%   90.02%   -0.07%     
==========================================
  Files         640      648       +8     
  Lines      188471   190963    +2492     
  Branches    36973    37433     +460     
==========================================
+ Hits       169802   171919    +2117     
- Misses      11382    11670     +288     
- Partials     7287     7374      +87     
Files with missing lines Coverage Δ
lib/v8.js 99.38% <100.00%> (+<0.01%) ⬆️
src/node_internals.h 81.03% <ø> (ø)
src/heap_utils.cc 79.60% <72.00%> (-0.54%) ⬇️

... and 116 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@juanarbol juanarbol added semver-minor PRs that contain new features and should be released in the next minor version. v8 module Issues and PRs related to the "v8" subsystem. labels Jul 6, 2025
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The BufferValue uses need to be cleaned up

@juanarbol
Copy link
Member Author

@addaleax @jasnell @Renegade334 PTAL :-)

if (WriteSnapshot(env, final_filename.c_str(), options).IsNothing()) return;

args.GetReturnValue().Set(
String::NewFromUtf8(isolate, final_filename.c_str()).ToLocalChecked());
Copy link
Member

@jasnell jasnell Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this is going to be a problem because it assumes the final_filename is utf8 encoded. Consider the following case:

const path = Buffer.from([0xe6]);  // latin 1 character
fs.mkdirSync(path);
const res = v8.writeHeapSnapshot(undefined, { path });
fs.readfileSync(res);  // fails! path doesn't exist

The reason fs.readfileSync would fail here is because the returned res will have interpreted the 0xe6 character incorrectly and will have replaced it with the unicode replacement char. The snapshot will have been written to disk correctly, but the returned path is wrong.

This likely needs to return the resulting path as a Buffer if the path was given as a Buffer and allow the API to accept an encoding option that optionally decodes the result as a string. But, that gets a bit awkward.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how picky this is, aaah, let me put the encoding option to make this a bit more consistent and leave utf8 as default encoding.

Nice catch, thanks! Anything else missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing that sticks out, but I should have time to take another pass through by end of day monday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. v8 module Issues and PRs related to the "v8" subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants