-
Notifications
You must be signed in to change notification settings - Fork 0
⚡ Bolt: [performance improvement] #388
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ## 2024-08-03 - [Avoid clone in pane_tree's push_tab_to_leaf] | ||
| **Learning:** In Rust performance optimization, avoid eagerly allocating cloned data (like `.clone()` on strings or identifiers) when passed into recursive search functions (e.g., `push_tab_to_leaf`). We can pass the value by ownership and return it back in the `Err` variant of a `Result` on a cache miss (e.g., `Result<(), SurfaceId>`). | ||
| **Action:** Use this pattern `Result<(), T>` instead of `bool` when transferring ownership into nested tree traversals. This allows the caller to reuse the same allocation for subsequent loop iterations without calling `.clone()`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -647,19 +647,26 @@ pub(super) fn push_tab_to_leaf( | |
| node: &mut PaneNode, | ||
| near_surface_id: &str, | ||
| new_tab_id: SurfaceId, | ||
| ) -> bool { | ||
| ) -> Result<(), SurfaceId> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The signature now returns AGENTS.md reference: AGENTS.md:L171-L171 Useful? React with 👍 / 👎. |
||
| match node { | ||
| PaneNode::Leaf { tabs, active } => { | ||
| if tabs.iter().any(|id| id == near_surface_id) { | ||
| tabs.push(new_tab_id); | ||
| *active = tabs.len() - 1; | ||
| true | ||
| Ok(()) | ||
| } else { | ||
| false | ||
| Err(new_tab_id) | ||
| } | ||
| } | ||
| PaneNode::Split { children, .. } => children | ||
| .iter_mut() | ||
| .any(|child| push_tab_to_leaf(child, near_surface_id, new_tab_id.clone())), | ||
| PaneNode::Split { children, .. } => { | ||
| let mut current = new_tab_id; | ||
| for child in children.iter_mut() { | ||
| match push_tab_to_leaf(child, near_surface_id, current) { | ||
| Ok(()) => return Ok(()), | ||
| Err(returned) => current = returned, | ||
| } | ||
| } | ||
| Err(current) | ||
| } | ||
| } | ||
| } | ||
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.
.julesroot bucketThis introduces a new top-level directory solely for a note about one private pane-tree helper; a repo-wide search finds no consumer or generator for it, and the repository previously removed a stale file at this same path. Keep implementation rationale beside the owning helper or omit this automation diary rather than creating a non-durable root content category.
AGENTS.md reference: AGENTS.md:L131-L131
Useful? React with 👍 / 👎.