Skip to content

Use instrument hooks #45

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 28, 2025
Merged

Use instrument hooks #45

merged 4 commits into from
Aug 28, 2025

Conversation

GuillaumeLagrange
Copy link
Contributor

@GuillaumeLagrange GuillaumeLagrange commented Jul 18, 2025 •
edited
Loading

Instrumented perf observation

  • some benches take a ~2μs hit, some take a ~6μs hit
  • one anomaly to investigate: test_iterative_fibo_100

Still ready for review

@GuillaumeLagrange GuillaumeLagrange marked this pull request as draft July 18, 2025 08:41
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1163-add-instrument-hooks-to-codspeed-node branch 4 times, most recently from 11d48ea to 2a2de7a Compare July 18, 2025 10:03
@GuillaumeLagrange GuillaumeLagrange marked this pull request as ready for review July 18, 2025 10:07
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jul 18, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #45 will degrade performances by 35.94%

Comparing cod-1163-add-instrument-hooks-to-codspeed-node (a3d2755) with main (8c86d0a)1

🎉 Hooray! codspeed-node just leveled up to 5.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 2 improvements
❌ 13 regressions
✅ 67 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
❌ RegExp#test 13.6 µs 20.2 µs -32.57%
❌ switch 1 23.6 µs 30.1 µs -21.64%
❌ test_recursive_fibo_10 21.7 µs 28.2 µs -22.92%
❌ test_recursive_fibo_10 39.5 µs 45.9 µs -14.06%
❌ test_recursive_fibo_10 21.5 µs 28.2 µs -23.62%
❌ test_recursive_fibo_10 39.5 µs 46.1 µs -14.16%
❌ RegExp#test 11.9 µs 18.6 µs -35.94%
❌ switch 1 24.1 µs 30.6 µs -21.4%
❌ RegExp#test 12 µs 18.8 µs -35.82%
❌ switch 1 24.1 µs 30.6 µs -21.27%
❌ RegExp#test 12 µs 18.7 µs -35.67%
❌ switch 1 24.3 µs 30.7 µs -20.87%
⚡ wait 500ms 12.3 ms 10.6 ms +16%
⚡ short body 295.8 µs 132.9 µs ×2.2
❌ wait 1ms 24.8 µs 31.1 µs -20.44%

Footnotes

  1. No successful run was found on main (a3d2755) during the generation of this report, so 8c86d0a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@GuillaumeLagrange GuillaumeLagrange marked this pull request as draft July 18, 2025 10:13
@GuillaumeLagrange
Copy link
Contributor Author

@not-matthias I'm looking into performance regression I'll ping you when it's full ready sorry

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1163-add-instrument-hooks-to-codspeed-node branch from 2a2de7a to 0711f7e Compare July 21, 2025 09:46
@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1163-add-instrument-hooks-to-codspeed-node branch 2 times, most recently from 0d7d691 to 712520f Compare August 27, 2025 15:21
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 27, 2025 •
edited
Loading

CodSpeed WallTime Performance Report

Merging #45 will degrade performances by 14.29%

Comparing cod-1163-add-instrument-hooks-to-codspeed-node (a3d2755) with main (8c86d0a)1

🎉 Hooray! codspeed-node just leveled up to 5.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 42 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ test_iterative_fibo_10 240 ns 216 ns +11.11%
❌ test_iterative_fibo_10 216 ns 252 ns -14.29%

Footnotes

  1. No successful run was found on main (a3d2755) during the generation of this report, so 8c86d0a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1163-add-instrument-hooks-to-codspeed-node branch 3 times, most recently from ad8b539 to 094b54b Compare August 27, 2025 16:34
@GuillaumeLagrange GuillaumeLagrange marked this pull request as ready for review August 27, 2025 16:38
not-matthias
not-matthias approved these changes Aug 27, 2025
View reviewed changes
Copy link
Member

@not-matthias not-matthias left a comment

Choose a reason for hiding this comment

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

Looks good! Please also set CODSPEED_PERF_ENABLED in CI, to ensure the integration works!

packages/core/src/native_core/instruments/hooks_wrapper.cc Outdated
Comment on lines 14 to 15
static InstrumentHooks *ensureInitialized() {
if (!g_hooks) {
InstrumentHooks *hooks = instrument_hooks_init();
if (hooks) {
g_hooks.reset(hooks);
}
}
return g_hooks.get();
}
Copy link
Member

Choose a reason for hiding this comment

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

This will add some performance impact on the StopBenchmark() path since we're checking first if the instrument hooks has been initialized. I've had a similar issue when adding instrument-hooks to codspeed-cpp.

We're initializing the global instance once, to avoid doing extra checks in the start/stop paths.

See:

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, since we're dealing with a JIT-compiled language here it should only have a very minimal effect. But maybe this can become an issue once the JS code is JIT-compiled and then directly calls this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'll test it out, it's maybe the source of some of the performance regressions that we observe. Won't be able to eliminate evrything but still.

@GuillaumeLagrange GuillaumeLagrange force-pushed the cod-1163-add-instrument-hooks-to-codspeed-node branch from dc40126 to cb3de54 Compare August 28, 2025 10:05
@GuillaumeLagrange GuillaumeLagrange merged commit a3d2755 into main Aug 28, 2025
14 checks passed
@GuillaumeLagrange GuillaumeLagrange deleted the cod-1163-add-instrument-hooks-to-codspeed-node branch August 28, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriencaccia adriencaccia adriencaccia approved these changes

@not-matthias not-matthias not-matthias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GuillaumeLagrange @adriencaccia @not-matthias