Skip to content
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

feat: add bun-cluster preset #3058

Closed
wants to merge 2 commits into from
Closed

Conversation

Saeid-Za
Copy link

@Saeid-Za Saeid-Za commented Feb 2, 2025

πŸ”— Linked issue

resolves #3057

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR tries to create a new preset to support clustering mode similar to node-cluster in bun.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Saeid-Za Saeid-Za requested a review from pi0 as a code owner February 2, 2025 21:01
@pi0
Copy link
Member

pi0 commented Feb 3, 2025

Thanks for the PR. Since bun already supports node:http and node:cluster, can you please try a benchmark to see if output of node-cluster on bun is faster or slower than this new bun-cluster?

(since it adds complexity to the codebase, I prefer to recommend node-cluster to bun users. it makes a difference in nitro v3 with native fetch support which we can consider)

@pi0 pi0 changed the title feat(preset): adding bun-cluster preset feat: add bun-cluster preset Feb 3, 2025
@Saeid-Za
Copy link
Author

Saeid-Za commented Feb 3, 2025

sure, I'll provide a benchmark soon, in the meantime, pooya, would you please expand on native fetch support?
Don't both bun and node support global fetch natively?
Thanks in advance πŸ™

@pi0
Copy link
Member

pi0 commented Feb 3, 2025

Yes indeed (and we do use native fetch for long time).

I meant Bun.serve({ fetch() => {} }) (as server interface). While it is more convenient, node:http implementation in Bun is sometimes faster.

@Saeid-Za
Copy link
Author

Saeid-Za commented Feb 4, 2025

I used oha to benchmark a simple GET endpoint built with both node-cluster and bun-cluster.
Here is the index.ts in routes:

let number = 1

export default eventHandler(async () => {
  return "Hello World! (request number: " + number++ + ")";
});

There were some unexpected/interesting results.

1. Using bun to run node-cluster : 31K Req/s

bun running node-cluster

2. Using Bun to run bun-cluster : 61K Req/s

bun running bun-cluster

3. Using node to run node-cluster: 72K Req/s

node running node-cluster


This means than if we set Bun running with bun-cluster as baseline:

  1. bun with bun-cluster is slower than node with node-cluster (14.55% slower)
  2. bun with bun-cluster is faster than running node-cluster preset (93.91% faster)

Additional Context

  1. The benchmark results might differ depending on your system's configuration. If possible, I kindly suggest testing it yourself
  2. I believe that using a simple GET endpoint for benchmarking does not accurately reflect real-world scenarios
  3. There were two runs in each scenario, one warm-up and the second for the data collection.
  4. I noticed that this route function was using async but did not need it, I also tested without async, there were no significant diffrence.

@Saeid-Za
Copy link
Author

Saeid-Za commented Feb 6, 2025

Based on the benchmark data, it appears that using the node-cluster preset with the Node.js runtime is more performant than Bun, even when utilizing Bun's own HTTP server and clustering mechanism. This contradicts claims that Bun's approach is faster.

Given this information, it seems that this pull request primarily adds code complexity without providing significant benefits. We could revisit this PR at a later stage after additional releases from Bun to see if future benchmark results change.

I will close this for now.

Thank you for your input!

@Saeid-Za Saeid-Za closed this Feb 6, 2025
@Saeid-Za Saeid-Za mentioned this pull request Feb 6, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add bun-cluster preset
2 participants