Repository navigation
node:v8, profilers: create heap snapshot and profile files with mode 0600 #45191
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
robobun
wants to merge
8
commits into
main
Choose a base branch
from
robobun/8e2178d1/heap-snapshot-file-mode
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
1d3cc08
node:v8: create the writeHeapSnapshot() file with mode 0600 like Node
robobun f8a8ea2
cli: create --heap-prof and --cpu-prof files with mode 0600 like Node
robobun d86a50d
Merge remote-tracking branch 'origin/main' into robobun/8e2178d1/heap…
robobun 799f203
test: check the writeHeapSnapshot() file mode in v8-module.test.ts
robobun 9053c12
test: check the heap profile file modes in tests of their own
robobun 0c20d3d
test: run the new file mode tests concurrently
robobun 52e9da8
Merge remote-tracking branch 'origin/main' into robobun/8e2178d1/heap…
robobun 9464713
ci: retrigger
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit (optional): the owner-only write helper duplicates
File::write_file_os_pathwith one constant changed and lives in the CPU profiler module, which the heap profiler then reaches into viacrate::bun_cpu_profiler::write_owner_only. Fix: put the mode where the file-writing concept is owned, for example amodeparameter onFile::write_file_os_pathin src/sys/file.rs:386 (its only other caller is src/install/bin.rs:1239, which would pass 0o664), or aFile::write_file_os_path_with_mode, and call that from both profilers.Why this was flagged
src/jsc/BunCPUProfiler.rs:82-85 adds
write_owner_only, whose body isopenat_os_path(Fd::cwd(), path, WRONLY|CREAT|TRUNC, 0o600)?.write_all(data); src/sys/file.rs:386-394write_file_os_pathis the same body with 0o664. src/jsc/BunHeapProfiler.rs:60,63,74,79 import it ascrate::bun_cpu_profiler::write_owner_only, making the heap profiler depend on the CPU profiler module for a generic file write. The repository review rules ask that new helpers go on the type that owns the concept and that an existing path be parameterized rather than cloned. Behaviour is otherwise unchanged versus the base; this is a placement and duplication concern only, which the PR description itself raises as an open question.Verification: nit. Acknowledged in diff: the PR description's "Question on the shape" section names this exact alternative. src/jsc/BunCPUProfiler.rs:82-85
write_owner_onlyis byte-for-byte the same shape as src/sys/file.rs:386-394write_file_os_pathwith0o600for0o664. The heap profiler reaches across modules for it at src/jsc/BunHeapProfiler.rs:60, 63, 74-77, 79-82.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not changed in this push. This is the "Question on the shape" in the PR body, and this thread stays open for a maintainer.
I compared the two shapes before I wrote the helper. They give the same syscalls, and the same code size on linux x64:
write_owner_onlyis 494 B, andFile::write_file_os_path::<Fd>goes from 500 B to 0 B because its other caller (src/install/bin.rs:1293) is Windows-only.The trade-off is this. With a required mode argument on
File::write_file_os_path, each caller must state a mode: open #41137 stops compiling until it does, and open #42626 only on Windows. With the local helper,src/sysand the 20 otherFile::write_file*call sites stay untouched, and the mode assertions inheap-prof.test.tsandcpu-prof.test.tscatch a profile that goes through another path.If a maintainer prefers the argument, the change is small: add
modetoFile::write_file_os_path, pass0o664insrc/install/bin.rs, pass0o600at the six profiler sites, and deletewrite_owner_only.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update after the merge with main (52e9da8). The size statement in my reply above no longer holds.
#46021 gave the CPU profile write a second caller, the sampling profiler report, and that caller keeps
File::write_file_os_path. So that function stays in the binary on POSIX, besidewrite_owner_only. On linux x64 release builds this PR now adds 1,024 B to.text(1,049 Bsizetext). Before the merge,.textdid not change.With a
modeargument onFile::write_file_os_path, one function would serve both modes. I did not build or measure that shape. The measured numbers are in the PR body under "Measurements after the merge". This thread stays open for a maintainer.