Avoid redundant re-hashing when squashing an already squashed branch #5867
Conversation
|
@aryairani The failing transcripts appear to be also failing on the latest trunk build. |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the discardHistory function to avoid redundant re-hashing when squashing branches that are already squashed. The main improvement is checking if a branch already has no history before performing expensive squashing operations.
- Introduces conditional logic to detect already-squashed branches and return them unchanged
- Adds helper functions that track whether any history was actually discarded to avoid unnecessary work
- Removes timing wrapper from entity insertion code in sync operations
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| parser-typechecker/src/Unison/Codebase/Branch.hs | Core optimization logic with new functions to conditionally discard history and track changes |
| unison-cli/src/Unison/Share/SyncV2.hs | Removes timing wrapper around entity insertion operation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Causal.One _ _ b0 -> do | ||
| let (Any changed, b0') = discardHistory0IfNecessary b0 | ||
| -- Avoid re-hashing things by returning the original if nothing actually changed | ||
| if changed | ||
| then (Any True, one b0') | ||
| else (Any False, b) |
There was a problem hiding this comment.
The do notation on line 281 is unnecessary since this is not a monadic context. Remove the do keyword for cleaner code.
| Causal.One _ _ b0 -> do | |
| let (Any changed, b0') = discardHistory0IfNecessary b0 | |
| -- Avoid re-hashing things by returning the original if nothing actually changed | |
| if changed | |
| then (Any True, one b0') | |
| else (Any False, b) | |
| Causal.One _ _ b0 -> | |
| let (Any changed, b0') = discardHistory0IfNecessary b0 | |
| -- Avoid re-hashing things by returning the original if nothing actually changed | |
| in if changed | |
| then (Any True, one b0') | |
| else (Any False, b) |
There was a problem hiding this comment.
I kind of like do let more than let .. in tbh
There was a problem hiding this comment.
Yeah I strongly agree, I add redundant do's all over the place because it prevents reformatting everything when you add another line.
I also despise when a PR review is just a bunch of redundant and inconsequential syntax tweaks someone arbitrarily wants you to do haha. Thanks Copilot...
There was a problem hiding this comment.
In the initial comment there's a table with a one-line summary of the changes to each file; that seems potentially useful.
I guess unsurprisingly, you can give the LLM instructions on how to conduct the review, that's cool too, in principle.
|
Sorry for using your PR to try out the Copilot review; checking now to see how horrible it may have been |
Overview
discardHistoryand friends was squashing branches in such a way that we'd always rehash them.This change avoids redundant re-hashing where possible.
This speeds up
pull.without-history; maybeupdateandlib.installin very specific situations.For Cody's situation, this speeds it up from 29s -> 2.6s; still too slow, but much better!
Implementation notes
This changes it so if a branch is already squashed we return it as-is from
discardHistory, which avoids memory redundancy and, more importantly, avoids re-hashing the whole subtree which is very expensive on branches with tons of dependencies.Test coverage
I tested performance on this issue Cody opened:
fixes #5866
Loose ends
Everywhere using
discardHistoryshould maybe just be usingsquashCausalwhich can use the squash-result table for caching, but that's all V2 machinery so it may take a bit more finesse to add that; I'll take a look in a separate change.