Skip to content
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

Vastly speed up ancestry check by not re-crawling duplicate histories. #4753

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Mar 6, 2024

Overview

Noticed when doing fast-forward detection in PG that base only has ~500 history nodes at the top level, so was curious why it was infeasible to do full ancestor detection; turns out that with UNION instead of UNION ALL we don't cull duplicates, meaning every time we would merge it'd crawl the entire history again 🙃 .

It's unintuitive, because you'd think UNION ALL is correct since the root causal shouldn't ever appear in the recursive tail, but it appears that's just not how UNIONs work in a WITH RECURSIVE CTE 🤷🏼‍♂️

Should speed up merges since this affects lca, but we can also make lca much faster, split that off into its own PR since it's a bigger change that I don't have time to test thoroughly right now: #4754

Implementation notes

UNION ALL -> UNION;
speeds this up by infinity percent (I never waited it to finish before, now it finishes instantly).

Test coverage

Did some sqlite tests and it should work.

Loose ends

While digging around I noticed that in https://github.com/unisonweb/unison/blob/trunk/parser-typechecker/src/Unison/Codebase/Causal.hs there are many functions which are computing predecessors and 'before' checks on in-memory haskell objects, these would be better on memory and probably much faster if they just used SQLite directly. Probably something for @mitchellwrosen and @tstat to look at as part of the merge rewrite 😄

See #4754

@ChrisPenner ChrisPenner mentioned this pull request Mar 6, 2024
@ChrisPenner ChrisPenner marked this pull request as ready for review March 6, 2024 19:57
@aryairani
Copy link
Contributor

aryairani commented Mar 8, 2024

Just curious if we have any existing / regression tests that exercise this?

@aryairani aryairani merged commit de8a7ca into trunk Mar 8, 2024
7 checks passed
@aryairani aryairani deleted the cp/speed-up-before-check branch March 8, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants