Add a cache mechanism for the PPX phase

[pitag-ha]

There are three different cache mechanisms for the typer phase, but none for the parsetree-related phases. This commit adds a simple cache for the reader phase and for the PPX phase, the important one being the latter.

Some things that might call your attention are:

• It's a very simple cache: 1. it's an all-or-nothing cache as opposed to the more elaborate typedtree cache. 2. As opposed to file caches, it doesn't have any clearance.
• I'm using a reference to hold the old parsetree in memory. Let me know if you'd like me to use a different data type!
• The cache for the PPX phase depends on the reader phase: the cache gets invalidated (among others) by source code changes as opposed to parsetree changes. The reason for that choice is that it's way simpler to digest the source code (string) than the parsetree. I pass that information through the mpipeline from the mpipeline reading step to the mpipeline PPX step. That has made the source component of the mpipeline a bit ugly: I've turned it from a tuple to a triple. Let me know if you want me to pass the information in a different way to the PPX step or if you want me to make a record out of that triple!
• I've extended the File_id module with a get_res function. The reason for that is that when in doubt whether the cache should be invalidated or not, it's better to invalidate it IMO: correctness over performance IMO. The case of an error while computing File_id is definitely a reason to doubt whether the cache can be used. However, that has made this cache incoherent with the file caches, which go ahead with a generic file id when a file id couldn't be computed. Let me know if you want more coherence here!

[rgrinberg]

Is there a way to manually coontrol this cache or toggle it somehow? I'm concerned that it isn't correct for ppx that relies on preprocessor_deps.

[pitag-ha]

That's a very good point, @rgrinberg. Thanks!

Do you mean toggling the cache from the outside, i.e. by the instance that's calling Merlin? How would that instance know whether there are preprocessor dependencies?

Maybe instead of toggling it from the outside, another option could be the following (let me know if that's already what you had in mind). We could extend the dot-merlin-reader to capture information about preprocessor_deps. I guess, for that we'd also need to extend the dune command (or the equivalent in a different set-up than dune), that feeds that information to the dot-merlin-reader. Is that right?

So for new versions of dune/jenga, Merlin would have information about preprocessor dependencies in its configuration. Then, the PPX mechanism could be as follows: does the configuration have information about the preprocessor dependencies? If no, no PPX cache. If yes, cache which takes preprocessor dependencies into account.

What do you think? Is that what you had in mind?

[rgrinberg]

Do you mean toggling the cache from the outside, i.e. by the instance that's calling Merlin? How would that instance know whether there are preprocessor dependencies?

I meant just being able to turn it off or control it when using merlin as a library. In lsp, I would use dune rpc to tell merlin when a preprocessed AST would be invalidated.

. We could extend the dot-merlin-reader to capture information about preprocessor_deps. I guess, for that we'd also need to extend the dune command (or the equivalent in a different set-up than dune), that feeds that information to the dot-merlin-reader. Is that right?

That would be very difficult because it would mean re-implementing dune's DSL specification for deps inside merlin.

[pitag-ha]

In lsp, I would use dune rpc to tell merlin when a preprocessed AST would be invalidated.

That's interesting! Thanks for explaining. We also need a way to make this work correctly in a different context than lsp, though. I'm not very familiar with the different communication layers that are done in different contexts for Merlin to get all its config information. I'll ask @voodoos about that and come back to you.

[ddickstein]

If preprocessor_deps aren't used often, maybe it would be easier to just say cache if the configuration specifies that there are no preprocessor dependencies, otherwise (if there are or if the configuration doesn't specify) don't cache. At least as a first pass.

[rgrinberg]

This PR reminded me of alternative method to speed up ppx that works without relying on cache hits (we won't get the cache hits in many cases). One of the reasons why ppx (and any other preprocessor) is slow is the cost of launching an external process every single time the input changes. I bet we could speed up preprocessing materially if we kept the ppx executable persistent, and just dispatched pp requests to it. It would require some changes to ppxlib to allow drivers to run in a --server mode, but it would be fairly universal.

Even in this server mode however, we'll need some co-operation from dune to re-run the server whenever the binary changes. On the lsp side, I have no problems with managing such ppx subprocesses. But perhaps it wouldn't work well with merlin?

In any case, I think it does make sense to make merlin a little more flexible and allow callers to customize how they would like to run preprocessors.

[voodoos]

A server mode for the driver would be a nice feature. Additionally the ppxes currently write the ast to a file on disk that is then read by merlin. Skipping that intermediary file might also speed up things a little ?

[pitag-ha]

@ddickstein, yes, I also think that's a good idea, for sure "as a start" and possibly permanently. In fact, that's what I understood @rgrinberg was referring to.

About the "speeding up the PPX phase by improving the interaction between Merlin and the PPX driver" idea: there are three different aspects to that point. As you, @rgrinberg, have pointed out, spawning a new process each time is time-consuming. As you, @voodoos, have pointed out, passing the AST between Merlin and the PPX process via disc is time-consuming. And the third one is that Merlin marshalling and the PPX process unmarshalling (and later vice-versa) is time-consuming. For the first, we can have the PPX driver run in server mode. For the second, we could pass the AST via a serial stream or similar. For all three of them, including the third, we could not do those two things and instead embed a PPX driver into Merlin that dynamically links the PPXs (that driver could be similar to the one utop uses). The latter also has timing downsides, though, so I'd need to find out what the time pay-off would be.

I've been analyzing some Merlin latency data during the last weeks, though, and I think there are several far more impactful things we can do currently to improve the PPX phase latency. One is that several AST passes could/should be gotten rid of, particularly for any Merlin query different than errors. And the other one is a cache for the PPX phase.

To give an idea about why I think those things are more impactful than improving the interaction between Merlin and the PPX driver: I had a look at the system time Merlin and its child process spend during the PPX phase, and it's marginal: I haven't found a case in which it was over 20 msecs, yet. In the cases in which the PPX phase is a bottleneck, it spends up to 500 msecs. And that without a cache, so it does so every single time for every single query.

[pitag-ha]

Btw, @rgrinberg, I'm very curious about your side-note

we won't get the cache hits in many cases

Quantitatively, what do you have in mind when you say "in many cases"? You have a way better overview of Merlin/LSP user behavior than me, so it would be interesting for me to understand what you have in mind (this is a very simple first step for the cache. improving it is one of the many things I might do next to improve Merlin performance).

Simplifying (quite) a bit, I thought that we could distinguish between a user doing code analysis and a user coding. For a user doing code analysis, there will be lots of type-enclosing queries and some locate queries. The cache will only be missed for the first query after a locate into a different file (depending on the editor, that will either be errors or the next code analysis query by the user). For a user who's coding, the cache hits depend on how many queries a user makes in between modifying the code. Also, depending on the editor, all the complete-prefix queries while writing would be cache misses. Is the complete-prefix situation the situation or one of the situations you had in mind?

[rgrinberg]

For all three of them, including the third, we could not do those two things and instead embed a PPX driver into Merlin that dynamically links the PPXs (that driver could be similar to the one utop uses)

Is that really what utop does? I'm pretty sure it just uses compiler-libs which executes the external processes.
In any case, dynlink is very complex and brittle. I personally wouldn't consider it.

Is the complete-prefix situation the situation or one of the situations you had in mind?

You have the right idea. When the user is actively writing code, we can't rely on cache hits. As you've mentioned, providing completion will be triggered heavily in this phase, so we'll be doing a lot of preprocessing without a lot of cache hits.

[rgrinberg]

For the second, we could pass the AST via a serial stream or similar.

Wouldn't the server mode already cover that? We'd be passing the AST over the socket. This should be faster than using a temporary file.

[pitag-ha]

Is that really what utop does?

I do think that that's what utop does for derivers.

I'm pretty sure it just uses compiler-libs which executes the external processes.

That (or something similar) is what it does for PPXs registered with (kind ppx_rewriter): it simply creates one different process for each PPX. However, for PPXs registered with (kind ppx_deriver) the situation is different: derivers need a common driver with information about all registered derivers to know whether a used deriving annotation is supported or not. So, utop uses the ppx_deriving driver for that, which dynlinks the derivers. At least, that's my understanding. If you still think I'm wrong, let me know! I don't discard it. In any case:

In any case, dynlink is very complex and brittle. I personally wouldn't consider it.

Not considering this sounds good to me. I was considering it before you mentioned the idea of launching Ppxlib in server mode. However, I think the server mode idea is better! Also about that:

Wouldn't the server mode already cover that? We'd be passing the AST over the socket. This should be faster than using a temporary file.

Good point!

But in any case, as I've mentioned before, I think we have lower-hanging fruits for now. So, to come back to the cache:

You have the right idea. When the user is actively writing code, we can't rely on cache hits. As you've mentioned, providing completion will be triggered heavily in this phase, so we'll be doing a lot of preprocessing without a lot of cache hits.

Ok, providing completion is indeed a problem we'll need to look into separately. That problem also depends on the editor I guess. Btw, that reminds me of a question I have: does the VSCode plug-in or the lsp-server in general have a way to cancel queries?

[rgrinberg]

does the VSCode plug-in or the lsp-server in general have a way to cancel queries?

The LSP protocol supports the client cancelling a request whenever the client is no longer interested in the response. It is relevant to ppx as well. For example, if the client requests diagnostics and immediately edits the document, the diagnostics request would be cancelled and we'd do well to stop whatever preprocessing is going on. Unfortunately, that means sending a signal to stop the ppx driver process, usually followed by re-running it on subsequent requests. That's all pretty inefficient, and we could do better without all these processes.

[pitag-ha]

The LSP protocol supports the client cancelling a request whenever the client is no longer interested in the response.

Cool! Does VSCode ever cancel old complete-prefix or other queries? I mean the situation where a new query comes in before the last query has replied.

[rgrinberg]

I'm not sure unfortunately. I would hope so.

I would also not recommend relying on this cancellation mechanism for performance. It can definitely help us, but unfortunately not all editors (their corresponding lsp clients) have it and their implementations are not really consistent with each other.

[TyOverby]

I'm in favor of @ddickstein's suggestion to ignore the cache if preprocessor_deps is used due to how infrequently it appears.

This "ppx server" idea seems great too - for files that are being actively edited, it'll be necessary to reduce the cost of running necessary ppxes - but it's not going to be easy; some unknown number of ppxes use global state under the assumption that it's going to be invoked on a single ml/mli file at a time.

[pitag-ha]

I'm in favor of @ddickstein's suggestion to ignore the cache if preprocessor_deps is used due to how infrequently it appears.

💯
I'll do that as soon as I'm back from Easter vacation.

some unknown number of ppxes use global state under the assumption that it's going to be invoked on a single ml/mli file at a time.

Hmm, this might be important for me to clarify, since I might be missing something. Could you explain this a bit more? I'm not sure what you mean by the "assumption that it's going to be invoked on a single ml/mli file at a time.". Isn't the PPX driver always just invoked on the AST (ml/mli file) of the current buffer?

Edit:

Nvm my question. It's clear and a very good point!

End edit

Btw, I also think that if we make the PPX driver run in server mode, we can later improve the performance of the PPX phase significantly: we can make the PPX driver conscious of whether all the PPXs it's linking are "state-less". In the case of a state-less driver, we could invoke the driver on each structure/signature item separately (thanks to the server mode) instead of invoking it on the whole AST. So we could implement a structure/signature item based PPX cache, which keeps track of all expanded items and only re-expands the ones that were modified. That cache would even be more powerful than the typer cache of the current buffer,, since it really only need to re-expand the modified items, whereas the typer cache needs to re-type all items from the first modified item on. I haven't thought this idea though yet, though. It just came to my mind when reading about the server idea. Does it go in the same direction as what you have in mind, @TyOverby?

[pitag-ha]

I've made the cache togglable now. @Ulysse, could you review when you have a moment?

Btw, there seem to be two different workflows to add new config values to merlin: via a new config directive, such as exclude-query-dir; or simply putting it into the generic bucket of flags, such as -ocamllib-path, -log-file etc. The first seemed quite involved, and the latter was extremely simple, so I've done the latter for now. Does that sound good? And also: do you know when it's necessary/advised to add new dedicated directives?

Also, I've decided to dump the new config value "use-ppx-cache" when someone runs dump-configuration. Most config values are dumped, but some aren't (such as config_path and exclude_query_dir).

[pitag-ha]

Btw, I forgot to mention: I've tested enabling the cache on a project with a .merlin-file by adding the new FLG -use-ppx-cache manually, and that works. When/if we merge this, we'll need to add support to dune, jenga and lsp-server to turn on the PPX cache for us whenever no PPX dependencies are used.

[voodoos]

Btw, I forgot to mention: I've tested enabling the cache on a project with a .merlin-file by adding the new FLG -use-ppx-cache manually, and that works.

Could you write a test illustrating the cache behavior in the test suite ?
Maybe showing that Merlin gave the same answer if the cache was active but the ppx add a hidden dependency that changed.

Note about test organization: so far the testsuite has only one test for ppxs named with-ppx.t. It is a directory test. I think we should rename that test typed-holes.t and make with-ppx a normal directory with all the tests depending on ppxlib. So that we would have test-dirs/with-ppx/typed-holes.t, test-dirs/with-ppx/your-new-test.t etc.

[pitag-ha]

Maybe showing that Merlin gave the same answer if the cache was active but the ppx add a hidden dependency that changed.

With "hidden dependency", are you referring to PPX dependencies? Whether PPX dependencies influence the PPX cache, doesn't depend on Merlin with this implementation, but on the build system. Merlin only knows whether the cache is disabled or enabled, not why: the PPX cache is disabled by default and can be enabled by the merlin config, i.e. by the build system (the build system should enable it if and only if there are no PPX dependencies).

What Merlin knows about is:

• is the cache disabled (that's the default)?
• was the PPX binary modified?
• were the args passed to the PPX binary modified?
• was the workdir modified?
• was the source code modified?

I can add tests to show that when any of the above points is the case, the parsetree is expanded instead of being pulled from the cache. That would be a different approach of test than what you've suggested (if I've understood you correctly): iiuc, you were suggesting an "add a test showing the limits of the current implementation -> fix the implementation -> see the fix in the test" approach. Is that right? I'm suggesting to just add a "test that the implementation is right" approach. Do you want me to add some test like that?

[voodoos]

I can add tests to show that when any of the above points is the case, the parsetree is expanded instead of being pulled from the cache. That would be a different approach of test than what you've suggested (if I've understood you correctly): iiuc, you were suggesting an "add a test showing the limits of the current implementation -> fix the implementation -> see the fix in the test" approach. Is that right? I'm suggesting to just add a "test that the implementation is right" approach. Do you want me to add some test like that?

Yes, we should have tests that show correct cache invalidation when it is expected that Merlin knows about it.

My suggestion was about showing that there is, in fact, a cache hit when it is no invalidated. I was thinking that we could trick Merlin in showing that it does use a cache by making a change that would not trigger cache invalidation because of the known limitation. But I guess we could simply print the log in the test...

Anyway, you should start by writing the tests you think make sense :-)

[voodoos]

Did a first rough review.

[pitag-ha]

I've added the tests now, both the ones I had in mind and the one you had in mind, @voodoos (I think. it's the one at the end of the new test file). The test structure has ended up different than you've suggested. I don't have an opinion on the structure tbh. So if you'd like it the way you've suggested, let me know! I think it should be very simple a sec to change it.

And to come back to whether to add the new use-ppx-cache config via the FLG directive or a new directive. I've done the first, but that might be a problem:

When using .merlin-files, then Merlin errors complains when there are unknown flags in the .merlin-file. However, it doesn't complain when there are unknown directives. So adding the new config via a flag will lead to incompatibility problems between new config workflows and old Merlins.

Does anyone know whether that's also a problem when using dune? I'd like to think that the config compatibility story is better for the Merlin-Dune communication, but I don't have any idea. @voodoos, do you know? Also, @rudi, I'm assuming that for ocaml-lsp this isn't a problem. If it is, please let me know!

[voodoos]

And to come back to whether to add the new use-ppx-cache config via the FLG directive or a new directive. I've done the first, but that might be a problem:

When using .merlin-files, then Merlin errors complains when there are unknown flags in the .merlin-file. However, it doesn't complain when there are unknown directives. So adding the new config via a flag will lead to incompatibility problems between new config workflows and old Merlins.

Good catch !

I actually thought that when using .merlin files unknown directives would be tagged as error by Merlin and that only when Dune is used those would be ignored.

I see three possible ways to go:

1. Make future versions of Dune with the new flag conflict with older versions of Merlin
2. Have Merlin ignore unknown flags when they come from Dune (and make future versions of Dune with the new flag conflict with versions of Merlin prior to the one that ignore unknown flags)
3. Use a new config directive

I am personally be in favor of 3 since we control which flags Dune sends to Merlin and if a wrong compiler flag is given it will be already told to the user by the compiler itself when building the project. This way future flag update won't always require a new conflict.

Also, what should happen if multiple ppxes are defined ? I guess we want to keep thing simple and have the flag apply to all of them ?

[pitag-ha]

I actually thought that when using .merlin files unknown directives would be tagged as error by Merlin and that only when Dune is used those would be ignored.

You're right! I've just double-checked and, when coming from a .merlin-file, unknown directives are indeed also tagged as errors. Now to make sure: You are sure that when coming from dune, they aren't tagged as errors, right? (I don't know how to check that myself without modifying dune).

About the 3 options you're proposing: I agree that 3. is the best option. I just hope it isn't too much work.

---

Also, what should happen if multiple ppxes are defined ? I guess we want to keep thing simple and have the flag apply to all of them ?

When saying "multiple PPXs", are you referring to multiple PPXs inside the same PPX driver (dune workflow) or are you literally referring to multiple PPX binaries (possible manual workflow / rescript workflow)? If it's the first, there's no choice. If it's the latter: I think it's nowadays unusual enough that it's ok to disable the whole cache when there's one PPX binary with PPX dependencies.

---

PD: I've made all the changes from your first review.

[voodoos]

Now to make sure: You are sure that when coming from dune, they aren't tagged as errors, right?

it should be yes, at least, that was the clear intent: https://github.com/ocaml/merlin/blob/master/src/kernel/mconfig_dot.ml#L238-L240

[voodoos]

A few more comments but the code actually looks quite good.
The test is also very clear and exhaustive 👍

[pitag-ha]

Thanks for the nice review comments, @voodoos! I've addressed them and have also changed the toggling config workflow: it's now via a new directive instead of a flag (to avoid messing with old merlin - new dune compatibility).

[voodoos]

Ok, can you rebase and clean the history a bit before the merge ? You should also add a changelog entry. Thanks !

[pitag-ha]

Cool! I've just rebased, squashed the commits, and added a changelog entry. After rebasing, I get a small formatting diff in tests/test-dirs/type-enclosing/inside-tydecl.t, also on the commit before my commit.