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

fix(node/bench): avoid affection of codspeed plugin in CI #706

Closed

Conversation

hyf0
Copy link
Member

@hyf0 hyf0 commented Mar 31, 2024

Description

codspeed has unkown problems for benchmarking program mixed with rust and js. For details:

We now use a hacky solution come up from @overlookmotel. We will first run a real benchmark to collect data, then run facade benchmark of vitest using the previous real benchmark data to pretend doing the bundling work.

Test Plan


Copy link
Member Author

hyf0 commented Mar 31, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hyf0 and the rest of your teammates on Graphite Graphite

Copy link

netlify bot commented Mar 31, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit b236b92
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/6609709a19ced40008f8f36b

Copy link

codspeed-hq bot commented Mar 31, 2024

CodSpeed Performance Report

Merging #706 will create unknown performance changes

Comparing 03-31-fix_node_bench_avoid_affection_of_codspeed_plugin_in_ci (b236b92) with main (ae9c7d0)

Summary

🆕 4 new benchmarks
⁉️ 4 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main 03-31-fix_node_bench_avoid_affection_of_codspeed_plugin_in_ci Change
⁉️ threejs 848.5 ms N/A N/A
⁉️ threejs-sourcemap 1.7 s N/A N/A
⁉️ threejs10x 9 s N/A N/A
⁉️ threejs10x-sourcemap 17.7 s N/A N/A
🆕 threejs N/A 52.4 ms N/A
🆕 threejs-sourcemap N/A 90.3 ms N/A
🆕 threejs10x N/A 546.4 ms N/A
🆕 threejs10x-sourcemap N/A 675.8 ms N/A

@hyf0 hyf0 force-pushed the 03-31-fix_node_bench_avoid_affection_of_codspeed_plugin_in_ci branch 2 times, most recently from 776678c to ffef85a Compare March 31, 2024 07:53
@hyf0 hyf0 force-pushed the 03-31-fix_node_bench_avoid_affection_of_codspeed_plugin_in_ci branch from ffef85a to edbde92 Compare March 31, 2024 08:06
@hyf0
Copy link
Member Author

hyf0 commented Mar 31, 2024

Give up so far. I can't make codspeed work right for benchmarking

function sleep(ms: number) {
  return new Promise((resolve) => setTimeout(resolve, ms))
}

@overlookmotel
Copy link
Collaborator

From my (incomplete) understanding, CodSpeed does not actually measure execution time (wall clock). It measures number of instructions executed and then converts that to milliseconds via some heuristic.

I imagine this is why setTimeout doesn't work as expected. No instructions execute, so no "time" is taken.

This is why in my hack on Oxc I have it execute meaningless instructions in a loop. That does get measured.

If you like, I can submit a PR applying the same technique?

@hyf0
Copy link
Member Author

hyf0 commented Mar 31, 2024

From my (incomplete) understanding, CodSpeed does not actually measure execution time (wall clock). It measures number of instructions executed and then converts that to milliseconds via some heuristic.

I imagine this is why setTimeout doesn't work as expected. No instructions execute, so no "time" is taken.

This is why in my hack on Oxc I have it execute meaningless instructions in a loop. That does get measured.

If you like, I can submit a PR applying the same technique?

Free to do it. Just looked your PR again. Nice solution, with this hack we could tracking benchmark rust and nodejs together. Solved my another requirement.

@hyf0
Copy link
Member Author

hyf0 commented Apr 1, 2024

@overlookmotel please be on hold. I'm thinking dropping codspeed using https://github.com/benchmark-action/github-action-benchmark.

@overlookmotel
Copy link
Collaborator

Sure. Personally, I really like Codspeed. It's just this one problem that is annoying. For benchmarking pure Rust code, it's excellent.

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.

2 participants