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

[hxb] load less dependencies during display requests #11650

Merged
merged 2 commits into from
May 2, 2024

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented May 2, 2024

Note that this still loads a lot of dependencies that are not really needed for display requests, but those are harder to skip without breaking everything.

This already reduces display requests time by 25~33% on my game project compared to latest dev (5.0.0-alpha.1+984c6e9).
Edit: on wartales that's a 0.7sec reduction per completion request 😁

Note that this still loads a lot of dependencies that are not really
needed for display requests, but those are harder to skip without
breaking everything.
@Simn
Copy link
Member

Simn commented May 2, 2024

The writer part looks good, this approach should be conservative enough because it captures all types potentially involved in an inferred signature.

This manual dependency business seems strangely named at the very least. The one in load_macro doesn't seem very "manual".

@kLabz
Copy link
Contributor Author

kLabz commented May 2, 2024

I've probably been too conservative on macro side too.. will check again
"manual" was meant to be for "added manually through macro APIs"

@Simn
Copy link
Member

Simn commented May 2, 2024

I think the one in load_macro is fine because we cannot track these dependencies from the expression alone.

Come to think of it, should m_manual_deps even be a thing at all? The implementation looks like this could become a flag on module_dep instead.

@kLabz
Copy link
Contributor Author

kLabz commented May 2, 2024

I have to admit this was implemented as a POC, and while I did rework/cleanup other parts, manual deps didn't get that treatment yet. The flag sounds like a good thing to try indeed

@skial skial mentioned this pull request May 2, 2024
1 task
@Simn
Copy link
Member

Simn commented May 2, 2024

No problem, I actually prefer slightly messy PRs over having a bunch of private branches that solve all problems once they're perfect.

@kLabz
Copy link
Contributor Author

kLabz commented May 2, 2024

I'm trying with this:

diff --git a/src/core/tType.ml b/src/core/tType.ml
index 30785bb82..2c47a1bb3 100644
--- a/src/core/tType.ml
+++ b/src/core/tType.ml
@@ -401,10 +401,15 @@ and module_def_display = {
        mutable m_import_positions : (pos,bool ref) PMap.t;
 }
 
+and module_dep_origin =
+       | MDepTyping
+       | MDepMacro
+
 and module_dep = {
        md_sign : Digest.t;
        md_kind : module_kind;
        md_path : path;
+       md_origin : module_dep_origin
 }

Which sets origin as MDepTyping for everything except the calls I was doing with ~manual_dependency:true. Maybe at some point we'll want to add more origins if that becomes of interest?

Not convinced by the naming, but also don't have much inspiration right now...
Edit: might at least change to MDepFromTyping / MDepFromMacro 🤔

@Simn
Copy link
Member

Simn commented May 2, 2024

Looks ok like that I think.

@kLabz kLabz merged commit 0adc110 into development May 2, 2024
99 checks passed
@kLabz kLabz deleted the hxb_reduce_dependencies_loading branch May 28, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants