-
-
Notifications
You must be signed in to change notification settings - Fork 115
fix benchmarks & codspeed integration #1034
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
Conversation
eeef244 to
7c1fda0
Compare
CodSpeed Performance ReportMerging #1034 will improve performances by ×7,900Comparing Summary
Benchmarks breakdown
Footnotes
|
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.
Pull request overview
This PR fixes benchmarks and integrates with CodSpeed by addressing naming conflicts and improving Rust FFI safety. The changes ensure that multiple benchmark executables can be built without symbol collisions, and enhance the safety documentation for Rust FFI functions.
- Introduces a macro-based prefix system for C++ benchmarks to avoid naming conflicts across multiple benchmark executables
- Updates Rust FFI functions to properly mark unsafe operations and adds safety documentation
- Updates ICU library dependencies from version 1.5.x to 2.1.x in the Cargo.lock file
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmarks/competitors/servo-url/lib.rs | Adds safety documentation and marks FFI functions as unsafe, improving Rust safety guarantees |
| benchmarks/competitors/servo-url/Cargo.lock | Updates ICU and related dependencies from 1.5.x to 2.1.x versions |
| benchmarks/benchmark_template.cpp | Introduces macro system (BENCHMARK_PREFIX) to namespace benchmark names and use RegisterBenchmark instead of BENCHMARK macro |
| benchmarks/bench.cpp | Defines BENCHMARK_PREFIX as Bench_ before including template |
| benchmarks/bbc_bench.cpp | Defines BENCHMARK_PREFIX as BBC_ before including template |
| benchmarks/CMakeLists.txt | Adds compile definitions for BenchData_ prefix to benchdata target |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return; | ||
| } | ||
|
|
||
| unsafe { drop(Box::from_raw(raw)) } |
Copilot
AI
Dec 9, 2025
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.
This unsafe block is redundant. The function is already declared as unsafe, so the entire function body is already in an unsafe context. You can remove the unsafe wrapper and just call drop(Box::from_raw(raw)) directly.
| unsafe { drop(Box::from_raw(raw)) } | |
| drop(Box::from_raw(raw)) |
| pub unsafe extern fn free_string(ptr: *const c_char) { | ||
| pub unsafe extern "C" fn free_string(ptr: *const c_char) { | ||
| // Take the ownership back to rust and drop the owner | ||
| let _ = unsafe { std::ffi::CString::from_raw(ptr as *mut _) }; |
Copilot
AI
Dec 9, 2025
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.
This unsafe block is redundant. The function is already declared as unsafe, so the entire function body is already in an unsafe context. You can remove the unsafe wrapper and just call std::ffi::CString::from_raw(ptr as *mut _) directly.
| let _ = unsafe { std::ffi::CString::from_raw(ptr as *mut _) }; | |
| let _ = std::ffi::CString::from_raw(ptr as *mut _); |

No description provided.