-
Notifications
You must be signed in to change notification settings - Fork 4
feat: more efficient diffs #134
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
Conversation
src/Share/Codebase/CodeCache.hs
Outdated
| expectTypeDeclsByRefIdsOf :: | ||
| (QueryM m) => | ||
| CodeCache scope -> | ||
| Traversal s t TypeReferenceId (V1.Decl Symbol Ann) -> | ||
| s -> | ||
| m t | ||
| expectTypeDeclsByRefIdsOf codeCache@(CodeCache {codeCacheCodebaseEnv}) trav s = do |
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.
What do you think of just calling directly into getTypeDeclsByRefIdsOf then unwrapping the Maybes into an error rather than copying the implementation? I think it'd make it easier to update these methods in the future :)
Same for the terms methods
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.
That seems fine; the inner database calls here are:
Codebase.loadV1TypeDeclarationsByRefIdsOf : Id -> Maybe DeclCodebase.expectTypeDeclarationsByRefIdsOf : Id -> Decl
so if we don't want to have a variant at the code cache level for an underlying call to Codebase.expectTypeDeclarationsByRefIdsOf, then we do have do duplicate the error it throws when a decl is missing. That is only a tiny amount of duplication, though. Still in favor of doing that instead of what's currently in the PR?
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.
Yeah a duplicate error message sounds fine, but avoiding duplicating the whole function would be worthwhile :)

Overview
This PR bumps the Unison dependency and integrates the new merge API, which is simpler and includes a "narrowing" step that allows us to avoid hydrating terms and types whose synhashes can't be different. This makes computing a namespace diff much more efficient, in the common case that most definitions are are not changed (where "changed" includes propagated updates).
This PR also optimizes a few other things related to computing and storing a namespace diff:
Some performance numbers from staging:
@unison/base, and 48 seconds to get a diff for a single update that has about 1000 transitive dependents.Next steps: I think it would be possible to further cut down on query times by batching better, if we want to pursue even faster diffs.
Note: don't merge yet! Plan: review this, then merge the Unison PR, then bump this PR's Unison dependency to point at that commit.