Add LRU to improve the memory usage of the indexer by Lucccyo · Pull Request #2050 · ocaml/merlin

Lucccyo · 2026-03-24T12:54:10Z

The index links are stored in a Hashtable, which grows without bound as we add new entries. This is using a lot of memory.
This PR replaces the use of a Hashtable with an LRU cache to bound memory usage, keeping only the most recently used indices and evicting the least recently used entry when the capacity is reached.

voodoos · 2026-03-24T14:22:42Z

(I did not review the tricky pointer arithmetic of the dllist yet)

art-w · 2026-03-31T11:00:22Z

-          | In_memory v -> write_child lnk schema v size ~placeholders ~restore
-          | On_disk _ ->
-            write_child lnk schema (fetch lnk) size ~placeholders ~restore)
+          | In_memory v | In_memory_c (v, _) ->


I'm curious if your tests ran into the In_memory_c case here? The current code is correct, but if this does happen in practice then In_memory_c would benefit from being translated into a pointed-index On_disk_ptr directly (since an In_memory_c is an in-memory value that was read from an existing file)

My test doesn't hit the In_memory_c case here.

art-w · 2026-03-31T11:07:37Z

+    in
+    List.iter
+      (fun (Cached (link, loc, store, schema)) ->
+        Cache.remove store.cache loc;


This remove doesn't look entirely correct, since we are loosing sharing as we never reintroduce lnk into the cache later if it turns out that offset was still useful (so we'll allocate new links for that same value). Does your benchmarks show that this remove is necessary? :)

Yes I agree.
On the benchmarks, removing the Cache.remove multiplies the max memory usage by around 1.1.

art-w · 2026-04-21T13:03:27Z

 module Cache = Hashtbl.Make (Int)

-type store = { filename : string; cache : any_link Cache.t }
+type store = { filename : string; id : int; }


It looks wrong to remove this cache, it's pretty essential :)

Lucccyo · 2026-04-22T08:56:03Z

+          | Serialized { loc } ->
+            (match Cache.find_opt store.cache loc with
+            | Some (Link (type b) ((existing_lnk, type_id') : b link * _)) ->
+              (match Type.Id.provably_equal type_id type_id' with
+              | Some (Equal : (a link, b link) Type.eq) ->
+                lnk := !existing_lnk
+              | None -> lnk := On_disk { store; loc; schema })
+            | None ->
+              Cache.add store.cache loc (Link (lnk, type_id));
+              lnk := On_disk { store; loc; schema })


@art-w What do you think? The cache keeps a trace (loc to link) of the serialized links.

yes it looks similar to the previous code :) (which we shouldn't need to update, so to keep the diff short it's best not to introduce minor differences)

art-w

Almost there! I have some minor requests to clean up the changes :)

art-w · 2026-04-22T10:33:29Z

+            | Some (Link (type b) ((existing_lnk, type_id') : b link * _)) ->
+              (match Type.Id.provably_equal type_id type_id' with
+              | Some (Equal : (a link, b link) Type.eq) ->
+                lnk := !existing_lnk


I misspoke the other day when I assumed you could remove all the Duplicate, this is a case where we are potentially leaking if we do not use them (the existing_lnk is tracked and released by the LRU, but the lnk copy isn't)

So I kept the Duplicate, but during my tests, they were never used.

art-w · 2026-04-22T10:34:50Z

+          match !lnk with
+          | Small v ->
+            schema iter v;
+            child_smalls:= (Value (Obj.magic v)) :: !child_smalls;


Can you run dune fmt, and also get rid of the magic now? :)

art-w · 2026-04-22T10:41:19Z

+          | Serialized { loc } ->
+            (match Cache.find_opt store.cache loc with
+            | Some (Link (type b) ((existing_lnk, type_id') : b link * _)) ->
+              (match Type.Id.provably_equal type_id type_id' with
+              | Some (Equal : (a link, b link) Type.eq) ->
+                lnk := !existing_lnk
+              | None -> lnk := On_disk { store; loc; schema })
+            | None ->
+              Cache.add store.cache loc (Link (lnk, type_id));
+              lnk := On_disk { store; loc; schema })


yes it looks similar to the previous code :) (which we shouldn't need to update, so to keep the diff short it's best not to introduce minor differences)

art-w · 2026-04-22T10:46:33Z

+val add_front : 'a t -> 'a * int -> 'a cell
+val discard_size : 'a t -> int -> 'a list
+val promote : 'a t -> 'a cell -> unit
+val get : 'a cell -> 'a


Is get really used?

Yes, it is, to return the value contained in the cell when we are fetching an In_cache value.

Lucccyo · 2026-04-22T12:50:52Z

@art-w I don't know how Duplicate should behave in the write`.
What we could want is to call ' write ' recursively on the original link, right?

Lucccyo · 2026-04-22T12:59:19Z

I pushed the review's resolution in a separate commit to make it easier to see the difference between before and after. They are going to be squashed and reorganized at the end.

art-w · 2026-04-22T15:00:23Z

+            | In_cache (_v, t, _children) ->
+              let Cached (_, loc, {filename; id; _}, _) = t.content in
+              lnk := On_disk_ptr { filename; id; loc }
+            | _ -> failwith "Granular_marshal.write: duplicate unexpected state")


For Duplicate, does the original code with write_child_reused work? The only new case should be In_cache, which is equivalent to an On_disk. (I believe it shouldn't be possible to have Duplicate (Small ... | In_memory ...), but let me know if that's not the case?)

I suggest going backward and reintroducing the Duplicate and *_reused, since they are linked, and we cannot remove *_reused while keeping the Duplicate.

Lucccyo · 2026-04-23T14:14:21Z

+      (match !parent with
+      | In_cache (_, _, small_poses) ->
+        let Value v = small_poses.(pos) in
+        Obj.magic v


@art-w How can we return properly v without the Obj.magic since it is existentially bound, and the typer rejects the case where we annotated v with type a like:

let (Value (v : a)) = small_poses.(pos) in

with the error on the part v : a:

File "src/index-format/granular_marshal.ml", line 192, characters 19-26: 192 | let (Value (v : a)) = small_poses.(pos) in ^^^^^^^ Error: This pattern matches values of type a but a pattern was expected which matches values of type $a1 Hint: $a1 is an existential type bound by the constructor Value.

This is the part where we use a Type.Id.t to convince the type-checker that the value has indeed the right type :)

… expired data.

…opened.

…store`.

… file.

…error message.

xvw reviewed Mar 24, 2026

View reviewed changes

Comment thread src/index-format/dbllist.ml Outdated

voodoos reviewed Mar 24, 2026

View reviewed changes

Comment thread src/index-format/lru.ml Outdated

Comment thread src/index-format/lru.ml Outdated

Comment thread src/index-format/lru.ml Outdated

Comment thread src/index-format/dbllist.ml Outdated

Comment thread src/index-format/dbllist.ml Outdated

Lucccyo force-pushed the lru_cache_index branch from 5bf10b0 to 4f75584 Compare March 31, 2026 09:16

art-w reviewed Mar 31, 2026

View reviewed changes

Lucccyo force-pushed the lru_cache_index branch 2 times, most recently from 7f47658 to 2e97c18 Compare April 20, 2026 14:01

art-w reviewed Apr 21, 2026

View reviewed changes

Lucccyo force-pushed the lru_cache_index branch from 2e97c18 to 516f769 Compare April 22, 2026 08:52

Lucccyo commented Apr 22, 2026

View reviewed changes

art-w suggested changes Apr 22, 2026

View reviewed changes

art-w reviewed Apr 22, 2026

View reviewed changes

Lucccyo commented Apr 23, 2026

View reviewed changes

Lucccyo force-pushed the lru_cache_index branch from c2bf2c4 to 33a4290 Compare April 24, 2026 09:48

Tim-ats-d added 13 commits May 4, 2026 12:21

Progress.

0bc5cd6

Clean up.

c45695f

Use shared cache sync with filesystem for ocaml-index file reading.

005a2c5

Use shared cache in Granular_marshal.read.

d1c23bd

Attach a random number to each index file in order to prevent reading…

97f314c

… expired data.

More precise error message.

2647727

Use 8 bytes to store index file random ID.

99c0146

Make is_on_disk behavior correct.

8731dfb

Use a random state instead of initialize a global one.

6802b4e

Delay pointing index check and check if a file descriptor is already …

b7aee1f

…opened.

Store ID in type store and delay the index ID check in `force_open_…

6247b2f

…store`.

Proper exception raising when attempting to consult an outdated index…

d403b4c

… file.

Catch Outdated_store exception in ocaml-index binary and provides an …

6230faf

…error message.

Lucccyo added 6 commits May 4, 2026 12:21

Add LRU to limit the memory usage

f825928

Removing compress optimization

dc9ac33

Add optim small_with_papa

3cf7589

review changes

1616aa9

printing in stderr

c86e8b3

minor fixes

9777008

Lucccyo force-pushed the lru_cache_index branch from 33a4290 to 9777008 Compare May 18, 2026 10:04

Fix functional value error

34e63f9

Conversation

Lucccyo commented Mar 24, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voodoos commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

art-w left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lucccyo commented Apr 22, 2026

Uh oh!

Lucccyo commented Apr 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants