perf: compute UIContext on prepare #29
Conversation
Summary of ChangesHello @satler-git, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on a performance enhancement for the UI rendering pipeline. By shifting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduce a new pub Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Batcher
participant Buffer
note right of Batcher: prepare phase
Caller->>Batcher: prepare(raw_buffer)
Batcher->>Batcher: map/filter -> Buffer<(UIContext, usize)>
Batcher->>Batcher: wrap -> Prepared<UIContext>
Batcher-->>Caller: Prepared<UIContext>
note right of Batcher: merge phase
Caller->>Batcher: merge(buf, prepared)
Batcher->>Prepared: into_inner()
Prepared-->>Batcher: Buffer<(UIContext, usize)>
Batcher->>Buffer: into_inner()
Buffer-->>Batcher: Vec<(UIContext, usize)>
Batcher->>Batcher: iterate & merge entries
Batcher-->>Caller: Result<bool>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/launcher/batcher.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a Prepared struct to encapsulate the buffer of UI context and indices, aiming to improve performance by computing the UI context during the preparation phase. The changes include modifications to the prepare and merge functions in src/launcher/batcher.rs, as well as the addition of an into_inner function to the Buffer struct in src/ui.rs. I have identified a potential issue with a panic that could be replaced with a more graceful error handling mechanism.
| if self.cushion_to_ui.is_none() { | ||
| panic!( | ||
| "{}", | ||
| eyre!( | ||
| "Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)" | ||
| ) | ||
| ); |
There was a problem hiding this comment.
The panic! macro immediately terminates the current thread. While this can be useful in development to catch unexpected states, it's generally better to handle errors more gracefully in production code. Consider returning a Result with an appropriate error message to allow the caller to handle the error case.
Also, the error message is very specific and includes implementation details. It would be better to provide a more general error message that doesn't expose internal workings.
if self.cushion_to_ui.is_none() {
return Err(eyre("UIContext is not set.")); // More general error message
}There was a problem hiding this comment.
@gemini-code-assist This branch is probably unreachable because the UI type parameter will not be decided without being set via the function(it also sets ctuf) or adding a type annotation. The user must know what they are doing.
There was a problem hiding this comment.
You're right that under normal, correct usage, this branch might be unreachable. However, in Rust, panic! is generally reserved for unrecoverable errors or programmer bugs that indicate a fundamental flaw in the program's logic. For situations where an API contract might be violated by the caller (e.g., cushion_to_ui not being set), returning a Result is the idiomatic way to signal an error that the caller can then handle gracefully.
Even if the user 'must know what they are doing,' a panic! will crash the thread, which is often undesirable in a larger application. Returning an Err allows the application to recover, log the issue, or present a user-friendly error message without terminating.
Additionally, the original error message still contains internal details. A more general message would be beneficial regardless of whether it's a panic! or an Err.
| if self.cushion_to_ui.is_none() { | |
| panic!( | |
| "{}", | |
| eyre!( | |
| "Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)" | |
| ) | |
| ); | |
| if self.cushion_to_ui.is_none() { | |
| return Err(eyre("UIContext is not set.")); // More general error message | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/launcher/batcher.rs (1)
473-482: Critical: Test will panic due to missingcushion_to_uiconfiguration.The
test_preparefunction will now panic becauseprepare()requirescushion_to_uito be set (see lines 183-190), but the test creates a defaultBatcherwithout configuring this field.Apply this diff to fix the test:
#[tokio::test] async fn test_prepare() -> Result<(), Box<dyn std::error::Error>> { let mut batcher: Batcher<i32, ()> = Batcher::default(); + batcher.cushion_to_ui = Some(Box::new(|_: &i32| ())); batcher.add_raw_source(Box::pin(tokio_stream::iter(vec![1, 2]))); let buf = batcher.prepare().await; - assert_eq!(buf.len(), 2); + assert_eq!(buf.into_inner().len(), 2); Ok(()) }Note: The assertion also needs to be updated since
prepare()now returnsPrepared<UIContext>instead ofBuffer<usize>.
🧹 Nitpick comments (2)
src/launcher/batcher.rs (2)
183-190: Consider usingensure!or plainpanic!for consistency.The
panic!(eyre!(...))pattern is unusual. Sinceeyre::ensure!is already imported and used elsewhere in the code, consider one of these alternatives:
- If this check is necessary: Use
ensure!directly and propagate the error- If this truly cannot happen at runtime: Use
debug_assert!or a plainpanic!with a string messageExample with
ensure!:- if self.cushion_to_ui.is_none() { - panic!( - "{}", - eyre!( - "Cushion to UIContext did not set. Did you set UI?(This error is probably not called because of the way Rust works!)" - ) - ); - } + ensure!( + self.cushion_to_ui.is_some(), + "Cushion to UIContext did not set. Did you set UI?" + );Note: If adopting this approach,
prepare()would need to returnResult<Prepared<UIContext>>.
336-336: Consider adding a clarifying comment for the nested unwrapping.The double
into_inner()call correctly extractsVec<(UIContext, usize)>fromPrepared<UIContext>, but the nesting may not be immediately clear to readers.Optional: Add a brief comment explaining the unwrapping:
+ // Extract Vec<(UIContext, usize)> from Prepared wrapper let v = from.into_inner().into_inner();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/launcher/batcher.rs(8 hunks)src/ui.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/ui.rs (1)
src/launcher/batcher.rs (1)
into_inner(121-123)
src/launcher/batcher.rs (1)
src/ui.rs (1)
into_inner(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: review (ubuntu-latest)
- GitHub Check: bench (ubuntu-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: lint (ubuntu-latest)
🔇 Additional comments (3)
src/ui.rs (1)
44-47: LGTM!Standard accessor pattern for consuming the wrapper and extracting the inner
Vec<T>. Thepub(crate)visibility is appropriate for internal use within the batcher.src/launcher/batcher.rs (2)
118-128: LGTM!The
Prepared<T>wrapper provides good encapsulation for prepared buffers. Thepub(crate)accessor methods are appropriately scoped for internal use.
287-309: LGTM! Performance optimization correctly implemented.The key change here moves
UIContextcomputation from merge-time to prepare-time, which aligns with the PR objective. Thecushion_to_uiclosure is now called duringprepare()(line 302), and the resulting(UIContext, usize)tuples are sorted before being wrapped inPrepared.
CodSpeed Performance ReportMerging #29 will improve performances by 82.46%Comparing Summary
Benchmarks breakdown
|
Summary by CodeRabbit