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

Define resolve_parent to memoize calls to prefix_substitution that were not (Dot cases) #1033

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Conversation

gpetiot
Copy link
Contributor

@gpetiot gpetiot commented Oct 31, 2023

No description provided.

Copy link
Collaborator

@Julow Julow left a 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

@gpetiot
Copy link
Contributor Author

gpetiot commented Nov 1, 2023

I've defined resolve_and_lookup_parent that calls resolve_parent (a glorified call to resolve_module, but the return type is not the same so it looks worth it to me to memoize it separately) and lookup_parent (pre-existing), and apparently dune build @runmdx now runs.

edit: my last commit just re-uses resolve_module instead of defining a new resolve_parent function, if that looks better let's squash the commits before merging.

Copy link
Collaborator

@Julow Julow left a 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?

src/xref2/tools.ml Show resolved Hide resolved
@gpetiot gpetiot marked this pull request as ready for review November 2, 2023 12:25
@Julow Julow added the no changelog This pull request does not need a changelog entry label Nov 17, 2023
@gpetiot gpetiot requested a review from Julow November 27, 2023 05:09
@Julow Julow requested a review from jonludlam November 27, 2023 09:20
Copy link
Collaborator

@Julow Julow left a 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.

Copy link
Collaborator

@Julow Julow left a 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.

@Julow Julow merged commit cb442aa into ocaml:master Feb 1, 2024
10 checks passed
@gpetiot gpetiot deleted the resolve-parent branch February 1, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-2.4 no changelog This pull request does not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants