Improve load path performance

[lpw25]

This PR makes 4 changes to speed up the handling of the load path:

1. It restores the use of Directory_content_cache for the load path -- which seems to have been removed by accident in the switch to 4.12.
2. It changes where 'Mocaml.setup_config` is called to ensure that it is only called once per command.
3. It uses a hash table for the load path instead of a reference to a map.
4. It reuses the previous hash table when the load path hasn't changed and none of it's directories' contents have changed.

On a randomly chosen command in a library with a large load path, each of these changes is worth around 0.1s. #1309 is also worth about 0.1s. Between them these changes take that command's runtime from 0.7s to 0.25s. (This is for the second call to the command, the initial call to the command is more like 1.9s vs 1.7s).

I also have a version of this branch for 4.11, which is the one I actually tested.

[let-def]

There a few things that could be further cleaned up but I am happy with the patch. Don't worry with the comments I left if you are busy, we can do this ourselves.

[lpw25]

I've updated the PR to match some adjustments that were made to the compiler PR, and I've addressed one of the comments.

[lpw25]

Rebased and CHANGES entry added

[let-def]

It is not correct to setup the loadpath only in the reader.

For instance, if we switch back-and-forth between two files with different build environments without changing them, the reader will be reused so Mocaml.setup_config will be invoked only the first time. The second time, the typer might run in the wrong environment.

Since it is Load_path.init that is expensive and only the typechecker needs it, I suggest to do it only when setting up the environment for the typechecker.

diff --git a/src/kernel/mocaml.ml b/src/kernel/mocaml.ml
index 77f1e2aa..6b4cd38d 100644
--- a/src/kernel/mocaml.ml
+++ b/src/kernel/mocaml.ml
@@ -25,12 +25,11 @@ let is_current_state state = match !current_state with
 
 (* Build settings *)
 
-let setup_config config = (
+let setup_reader_config config = (
   assert Local_store.(is_bound ());
   let open Mconfig in
   let open Clflags in
   let ocaml = config.ocaml in
-  Load_path.init (Mconfig.build_path config);
   Env.set_unit_name (Mconfig.unitname config);
   Location.input_name  := config.query.filename;
   fast                 := ocaml.unsafe ;
@@ -46,6 +45,11 @@ let setup_config config = (
   open_modules         := ocaml.open_modules ;
 )
 
+let setup_typer_config config = (
+  setup_reader_config config;
+  Load_path.init (Mconfig.build_path config);
+)
+
 (** Switchable implementation of Oprint *)
 
 let default_out_value          = !Oprint.out_value
diff --git a/src/kernel/mocaml.mli b/src/kernel/mocaml.mli
index ffb67072..3a8fb6d5 100644
--- a/src/kernel/mocaml.mli
+++ b/src/kernel/mocaml.mli
@@ -6,7 +6,8 @@ val with_state : typer_state -> (unit -> 'a) -> 'a
 val is_current_state : typer_state -> bool
 
 (* Build settings *)
-val setup_config : Mconfig.t -> unit
+val setup_reader_config : Mconfig.t -> unit
+val setup_typer_config : Mconfig.t -> unit
 
 (* Replace Outcome printer *)
 val default_printer :
diff --git a/src/kernel/mpipeline.ml b/src/kernel/mpipeline.ml
index f6add7e0..c9700e0d 100644
--- a/src/kernel/mpipeline.ml
+++ b/src/kernel/mpipeline.ml
@@ -156,7 +156,7 @@ let process
   let reader = timed_lazy reader_time (lazy (
       let lazy source = source in
       let config = Mconfig.normalize config in
-      Mocaml.setup_config config;
+      Mocaml.setup_reader_config config;
       let result = Mreader.parse ?for_completion config source in
       result, config
     )) in
@@ -169,6 +169,7 @@ let process
     )) in
   let typer = timed_lazy typer_time (lazy (
       let lazy { Ppx. config; parsetree; _ } = ppx in
+      Mocaml.setup_typer_config config;
       let result = Mtyper.run config parsetree in
       let errors = timed_lazy error_time (lazy (Mtyper.get_errors result)) in
       { Typer. errors; result }

Longer term, it might be better to think about a better way to do incrementality and rewrite this Load_path accordingly. But it will make maintenance more difficult, the current trade-off might not be so bad.

[lpw25]

Why is this still not merged?

[let-def]

I still had bug with it, I was able to track them down to the use of the hash table, but I am not able to explain where it is going wrong. When I stopped looking at that problem, I was thinking of rewriting parts of Load_path...
I will resume work on it on Wednesday, but the patch cannot be merged as it is.

Not yet ready according to last comment

[let-def]

Finally, I think I nailed the issue (I will test for a few days). 🎆
I wrote a bug post-mortem in the commit message:

When an inconsistent environment is detected (a loaded module changed,
directly or because the way its path is resolved changed), the local
store is reset.

This has the unfortunate side-effect of resetting the Load_path storage.

Before the Load_path module, all caching was done by merlin based on the
value of Config.load_path, outside of the Local_store.

The code has been wrong since the introduction of Load_path: its
internal state was always reset.
However, Merlin frontend was "defensively" repopulating it, which
explained some of the slowdown fixed by PR#1323: most of the time this
had no other effect than scanning the file system twice, but if the
state had been reset since last call, the cache would be repopulated.

The bug was particularly hard to track because there is a background job
in Merlin that remove old entries from the cache: if a file hasn't been
used for 5 minutes, its deps are removed from the cache and its
environment is considered inconsistent, even if nothing has changed.
We would then have to wait more than 5 minutes to reproduce this bug.

This can be tweaked by changing Server.server_accept loop in
ocamlmerlin_server.ml:

-      Mocaml.flush_caches ~older_than:300.0 ();
+      Mocaml.flush_caches ~older_than:15.0 ();

for instance, to set a 15 seconds cache.

Fundamentally, the issue is due to Local_store scope being limited to
the typechecker state, while the load_path was considered as part of the
configuration, at least originally.
That way, resetting the Local_store would set the typer in a fresh state
ready to type with the current configuration.
The introduction of Load_path moved the state out of the global Config
and to the Local_store. Now, resetting the typer also resets the load
path. The code that used to work would now typecheck in an empty
path/environment.

[let-def]

On my fork, I rebased this branch on top of current master for testing: https://github.com/let-def/merlin/tree/fast-load-path
I will update this PR later if it works as expected.

[let-def]

I fixed the few problems I had and it has now been running smoothly for two weeks.
I will merge the PR asap.