HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.14k stars 656 forks source link

[hxb] load less dependencies during display requests #11650

Closed kLabz closed 5 months ago

kLabz commented 5 months ago

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 :grin:

Simn commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

Simn commented 5 months ago

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

kLabz commented 5 months ago

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 :thinking:

Simn commented 5 months ago

Looks ok like that I think.