This is a quick benchmark for an apple to apple comparison with dora-rs#391
Merged
Conversation
We had a cascading error when the main thread would panic and drop the logger. This at least try to keep the logger in a consistent state.
Contributor
There was a problem hiding this comment.
Pull Request Overview
A new “cu_dorabench” example is added to provide an apple-to-apple benchmark comparison against dora-rs, along with enhancements to mutex handling in the unified logger.
- Introduces the
cu_dorabenchexample, including source/sink tasks, runner, log reader, config, and README - Updates
core/cu29_unifiedlogto replaceunwrap()on poisoned mutexes withexpectand guardedlock()handling - Adds the example to the workspace
Cargo.tomland defines a newscreamingprofile
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/cu_dorabench/src/tasks.rs | Implements DoraSource/DoraSink benchmark tasks |
| examples/cu_dorabench/src/main.rs | Sets up and runs the benchmark application |
| examples/cu_dorabench/src/logreader.rs | CLI to export logged messages |
| examples/cu_dorabench/src/lib.rs | Reexports tasks |
| examples/cu_dorabench/copperconfig.ron | Pipeline configuration |
| examples/cu_dorabench/build.rs | Emits LOG_INDEX_DIR env var |
| examples/cu_dorabench/README.md | Usage notes for the benchmark |
| examples/cu_dorabench/Cargo.toml | New example crate and dependencies |
| core/cu29_unifiedlog/src/lib.rs | Switches unwrap() to expect/guarded locks in MmapStream |
| Cargo.toml | Adds examples/cu_dorabench to the workspace and screaming profile |
Comments suppressed due to low confidence (1)
examples/cu_dorabench/src/main.rs:33
- The
debug!macro is used without importing it (e.g.,use log::debug;), which will cause a compile error or unresolved macro.
debug!("Application Ended: {}", error)
|
|
||
| fn process(&mut self, clock: &RobotClock, new_msg: Self::Output) -> CuResult<()> { | ||
| new_msg.metadata.tov = Tov::Time(clock.now()); | ||
| let buffer = self.pool.acquire().unwrap(); |
There was a problem hiding this comment.
Avoid using unwrap() on pool.acquire() to prevent panics—return an error via ? or handle the None case gracefully.
| fn process(&mut self, clock: &RobotClock, new_msg: Self::Output) -> CuResult<()> { | ||
| new_msg.metadata.tov = Tov::Time(clock.now()); | ||
| let buffer = self.pool.acquire().unwrap(); | ||
| buffer.lock().unwrap()[42] = 42; |
There was a problem hiding this comment.
[nitpick] The hard-coded index 42 is a magic number; consider defining a named constant or documenting its significance to improve readability.
Suggested change
| buffer.lock().unwrap()[42] = 42; | |
| buffer.lock().unwrap()[BUFFER_INDEX] = 42; |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
If you happen to be able to actually run theirs that would be even better.