dune build -w memory usage grows without bounds when using OCaml syntax in dune files

[copy]

Expected Behavior

dune build -w memory usage should stay below some upper bound over a longer period of time.

Actual Behavior

dune build -w grows without bounds.

Reproduction

Minimal example:

% cat test.ml 
% cat dune
(* -*- tuareg -*- *)

open StdLabels
open Jbuild_plugin.V1

let () =
  Printf.ksprintf send {|
(executable
 (libraries stdlib-shims)
 (name test))
|}
% cat dune-project 
(lang dune 3.0)
% dune build -w test.exe

In another terminal:

while true; do; echo >> test.ml; sleep 0.1; done

More realistic example:

• In any bigger project, run git clone https://github.com/ocaml/stdlib-shims.git && git clone https://github.com/mirage/bigarray-compat.git (I believe the presence of dune files containing OCaml syntax is sufficient to trigger their execution and this memory leak)
• Use dune build -w and make some normal edits
• Observe that the memory usage increases on every edit (in my project, about 25MB per build)

Specifications

• Version of dune (output of dune --version): 3.6.2
• Version of ocaml (output of ocamlc --version): 4.14.0 or 5.0
• Operating system (distribution and version): Linux 6.1.6

Additional information

dune --trace-file output:

Memtrace output (added to dune 3.6.2 in bin/main.ml):

[emillon]

Thanks for the great report and repro case. I was able to reproduce and bisect this down to d3296a6 (#5877). I'm not familiar enough with the Memo API to go further - @rgrinberg do you know what could be causing this?

[emillon]

I naively tried to replace ( == ) with structural equality and add ~cutoff arguments to both instances of Memo.create in Dune_load but this does not seem to fix this.

[rgrinberg]

Thanks for taking a look. Changing equality doesn't help it's only for hash collisions.

I think the way to go about it is just to change the memoization store to stop storing old values. We're currently using a hash table, but I suppose we only need the last value for every dune file path we evaluate.

[emillon]

Do you mean using Memo.create_with_store? I'll try to do that.

[rgrinberg]

Do you mean using Memo.create_with_store? I'll try to do that.

Yes. The idea is to use a Context_name.Map.t as a store and only store the last result. Note that you still need to take and make sure the last result is up to date wrt your input. But that's easy enough with structural/physical equality.

[emillon]

Before trying that I tried to print the sizes of the tables used by the two call sites of Memo.create in Dune_load and the tables do not have more than 1 entry. Is this expected? (I modified Memo.create to return the table, and inspect Table.length before each Memo.exec)

[rgrinberg]

Before trying that I tried to print the sizes of the tables used by the two call sites of Memo.create in Dune_load and the tables do not have more than 1 entry. Is this expected? (I modified Memo.create to return the table, and inspect Table.length before each Memo.exec)

That would be expected. The hashing function for this function is poor, so all the entries would go into a single bucket.

[emillon]

I don't think I understand how Table works. I thought that the hash part was just an optimization and that it allowed storing keys that have the same hash (but in a sort of linked list). What does length return? The number of buckets or the total number of bindings?

My assumption with this issue is that every time the file is modified, a new binding is added to the table, and the table is never cleared. This is what I wanted to check with length. Does that sound reasonable?

[Alizter]

Would we ever want to trim our memo table to fit some constraints? We could experiment with a cache replacement policy.

[rgrinberg]

What does length return? The number of buckets or the total number of bindings?

Never mind, it's the total number of bindings.

My assumption with this issue is that every time the file is modified, a new binding is added to the table, and the table is never cleared. This is what I wanted to check with length. Does that sound reasonable?

Yes, that confirms it.

[emillon]

We discussed this and yes there must be a growing table but we don't know which one.
I'm going to have a look using, the 3 following debugging techniques have been mentioned:

• do what I did but for other tables
• grow the heap to 2GB or similar and eyeball the heap
• use statmemprof.

[emillon]

I actually used another technique: I added the sizes of the memo tables to the trace-file output. It shows several growing tables:

• dir-contents-get0 (1 entry per run)
• execute-rule (8 entries per run)
• lib-instantiate (1 entry per run)

(for comparison, the same test on dune itself does not show any growing table)

I'll have a look at the input/outputs of these tables.

[emillon]

For some of these tables, the input is compared using physical equality (==) (ex: to compare Super_context.t). This means that the previous versions are kept in the table.

For rules (inputs of execute-rule) I'm surprised to see that all rules are kept, even the ones that correspond to former states of the dune file. What is the best way to fix this?

[rgrinberg]

You can try switching the store for libraries and see if that helps. We don't really need old libs and they're quick enough to construct.

For rules (inputs of execute-rule) I'm surprised to see that all rules are kept, even the ones that correspond to former states of the dune file. What is the best way to fix this?

That's not necessarily bad for rules. It's often the case that people go back and forth between a few states.