feat: take Bun profile #8515
Open
feat: take Bun profile #8515
Conversation
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
Summary of ChangesHello @twoeths, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR adds Bun runtime profiling capabilities, refactoring existing NodeJS profiling into a unified Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces profiling capabilities for Bun, which is a great addition. The refactoring to create a unified profileThread function is a good approach. I've found a bug in the NodeJS profiling implementation where the output directory and filename are not handled correctly after the refactoring. I've also included suggestions to fix this and to improve the readability of the new Bun profiling logic. Please see my detailed comments below.
Comment on lines
61
to
74
| async function profileBun(thread: ProfileThread, durationMs: number): Promise<string> { | ||
| const start = Date.now(); | ||
| let now = Date.now(); | ||
| while (now - start < durationMs) { | ||
| // biome-ignore lint/suspicious/noConsole: need to use console api to profile in Bun | ||
| console.profile(String(now)); | ||
| await sleep(BUN_PROFILE_MS); | ||
| // biome-ignore lint/suspicious/noConsole: need to use console api to profile in Bun | ||
| console.profileEnd(String(now)); | ||
| now = Date.now(); | ||
| } | ||
|
|
||
| return `Time taken to profile Bun ${thread} thread: ${now - start}ms`; | ||
| } |
There was a problem hiding this comment.
The while loop logic can be simplified for better readability. By calculating the number of profiling chunks upfront, we can use a for loop which makes the intent of the code more explicit.
async function profileBun(thread: ProfileThread, durationMs: number): Promise<string> {
const start = Date.now();
const profileCount = Math.ceil(durationMs / BUN_PROFILE_MS);
for (let i = 0; i < profileCount; i++) {
const profileLabel = `${thread}-${start}-${i}`;
// biome-ignore lint/suspicious/noConsole: need to use console api to profile in Bun
console.profile(profileLabel);
await sleep(BUN_PROFILE_MS);
// biome-ignore lint/suspicious/noConsole: need to use console api to profile in Bun
console.profileEnd(profileLabel);
}
const end = Date.now();
return `Time taken to profile Bun ${thread} thread: ${end - start}ms`;
}There was a problem hiding this comment.
it's not correct for big while loop, tried that with durationMs up to 1 epoch and it's >30s slippery
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8515 +/- ##
============================================
- Coverage 52.19% 52.19% -0.01%
============================================
Files 852 852
Lines 65054 65073 +19
Branches 4771 4774 +3
============================================
+ Hits 33955 33963 +8
- Misses 31030 31041 +11
Partials 69 69 🚀 New features to boost your workflow:
|
|
was able to take profile with NodeJS, no regression there |
| * If we increase this time it'll potentiall cause the app to crash. | ||
| * If we decrease this time, profile recorded will be fragmented and hard to analyze. | ||
| */ | ||
| const BUN_PROFILE_MS = 3 * 1000; |
There was a problem hiding this comment.
crash after 3 seconds? that sounds like definitely a bug in Bun
There was a problem hiding this comment.
@Jarred-Sumner not exactly right after 3s, but I tried with 6s and our beacon-node with Bun crashed. But it's not a blocker for main thread
do you know if Bun supports taking profile for worker threads? I cannot do that, the inspector shows nothing and I'm not able to select threads. Just want to know it's on your TODO's list or I should report a bug for it
just fyi in lodestar, we have a separate network thread, and another discv5 thread so it's quite important for us to know how Bun works there
Performance Report✔️ no performance regression detected Full benchmark results
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Motivation
Description
profileBunapi usingconsole.profile()apisdebug.bun.shwill take forever to loadprofileThreadas wrapper of eitherprofileNodeJSorprofileBundebug.bun.shweb page as the inspector, profile will be flushed from node to to the inspected and rendered live thereSteps to take profile
--inspectand look fordebug.bun.shloghttps://debug.bun.sh/#127.0.0.1:9229/0qoflywrwsocurl -X POST http://localhost:9596/eth/v1/lodestar/write_profile?thread=mainTimelinestab inhttps://debug.bun.sh/, checkCall treethereSample Profile
Timeline%20Recording%201 (4).json.zip