Use faster hash maps and precomputed hashes in hot paths - #15279
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR switches selected hot-path maps and sets to hashbrown/rustc-hash and adds Hashed<K> for precomputed-hash lookups. No approved spec context was present, and the security pass did not find a diff-backed security issue.
Concerns
- The new
Hashed<K>tests include one case that combines multiple independent behavior contracts, contrary to the repo's unit-test guidance. AIBlock::handle_updated_outputstill rebuilds the complete action-ID set inside the per-action loop, so the touched hot path remains unnecessarily O(n²).
Verdict
Found: 0 critical, 1 important, 1 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| } | ||
|
|
||
| #[test] | ||
| fn derefs_and_compares_as_the_key() { |
There was a problem hiding this comment.
|
|
||
| for action in output.actions() { | ||
| let new_action_ids: HashSet<AIAgentActionId> = | ||
| let new_action_ids: FxHashSet<AIAgentActionId> = |
There was a problem hiding this comment.
💡 [SUGGESTION] This still rebuilds the full action-ID set inside the per-action loop, making the touched hot path O(n²). Collect it once before for action in output.actions() and reuse it.
- Add a generic Hashed<K> utility in warp_util that pairs a key with its precomputed hash, so repeated lookups against the same hasher (e.g. hashbrown's raw_entry().from_hash) can skip rehashing. - Use Hashed<TaskId> for TaskStore's root_task_id, and switch its backing map to hashbrown::HashMap to access raw_entry. - Switch AIConversation's tasks_by_id map to hashbrown::HashMap. - Swap std HashMap/HashSet for FxHashMap/FxHashSet (rustc-hash) in a few other hot paths: AIBlock's requested_action_ids, BlockList's block_id_to_block_index, and AppContext's window/cursor maps. - Declare hashbrown as a workspace dependency and reference it (and rustc-hash) from app/Cargo.toml via workspace = true for consistency. Co-Authored-By: Warp <agent@warp.dev>
df3ed9c to
2ed9114
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
1 participant
Description
Stacked on #15162. Follow-up performance pass that reduces hashing overhead in a few hot paths:
Hashed<K>utility (crates/warp_util/src/hashed.rs) that pairs a key with its precomputed hash, so repeated lookups against the same hasher (e.g.hashbrown'sraw_entry().from_hash(..)) can skip rehashing.TaskStore::root_task_idis nowHashed<TaskId>, andTaskStore's backing map switched tohashbrown::HashMapto useraw_entry.AIConversation'stasks_by_idmap switched tohashbrown::HashMap.HashMap/HashSetforrustc-hash'sFxHashMap/FxHashSetin a few other hot paths that don't need DoS-resistant hashing:AIBlock::requested_action_ids,BlockList::block_id_to_block_index, andAppContext's window/cursor-position maps. Since these keys are not from user input, we do not need DoS resistant hashing and can instead opt for better performance offered byrustc-hash.hashbrownas a workspace dependency and referenced it (andrustc-hash) fromapp/Cargo.tomlviaworkspace = truefor consistency with the rest of the manifest.hashbrownwould not change the behaviour, as call stack tracing has already confirmed that, at least in the Rust standard library we use Mac, it is already the underlying implementation ofHashMap, so using it to replacestd::collection::HashMapsimply allows us to access more API with the same underlying implementation.No behavior changes are intended; this is purely a performance/internal refactor.
Linked Issue
Testing
Hashed<K>incrates/warp_util/src/hashed_tests.rs.ai::agent::task_store(32 tests),ai::blocklist::block(104 tests),terminal::model::blocks(65 tests) — all passing../script/formatandcargo clippyon the touched crates (warp,warp_util,warpui_core) with no new warnings../script/runAgent Mode
CHANGELOG-NONE
Co-Authored-By: Warp agent@warp.dev