-
Notifications
You must be signed in to change notification settings - Fork 94
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
Define resolve_parent to memoize calls to prefix_substitution that were not (Dot cases) #1033
Conversation
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.
This has a very large impact on the slowest module to compile (Html_sigs
in tyxml), which contains a large number of types.
src/xref2/test.md
is failing and I don't understand why at the moment (dune build @runmdx
). t
no longer resolves in this example:
module A : sig
module M : sig type t end
module N = M
module O = N
end
type t = A.O.t
I've defined edit: my last commit just re-uses |
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.
This shouldn't be a draft anymore!
Can you rebase on top of #1034 to see the result on bigger libraries?
…ion that were not (Dot cases)
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.
This seems to be a general improvement to the correctness and performances. The double lookup reduces the amount of code and the number of caches, which seems to be the right direction.
I'd rather merge this before more work can be done on this part of the code.
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.
There have been concerns about the double lookup but this PR is already an improvement. More improvements should be made on top and should not block this.
Let's merge before it rots.
No description provided.