irmin-pack: no mmap syscall #2232

art-w · 2023-03-30T15:38:13Z

Since the mapping file is read-only, we can simulate the mmap in userland by lazily reading chunks on demand (and avoid the file descriptor leak from mmaped bigarrays..) There are a bunch of optimizations that we could try here, but I'm curious how the benchmarks are impacted by this change before going too deep into this rabbit hole :)

(also added the expected mapping_size in the control file as a consistency check to detect a partially written mapping, after a gc or a snapshot)

codecov-commenter · 2023-03-30T15:57:25Z

Codecov Report

Merging #2232 (85d706e) into main (63e6e74) will increase coverage by 0.04%.
The diff coverage is 76.62%.

❗ Current head 85d706e differs from pull request most recent head ccaa77e. Consider uploading reports for the commit ccaa77e to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2232      +/-   ##
==========================================
+ Coverage   67.97%   68.02%   +0.04%     
==========================================
  Files         136      136              
  Lines       16564    16587      +23     
==========================================
+ Hits        11260    11283      +23     
  Misses       5304     5304

Impacted Files	Coverage Δ
src/irmin-pack/unix/gc_worker.ml	`3.55% <0.00%> (-0.05%)`	⬇️
src/irmin-pack/unix/store.ml	`65.43% <ø> (ø)`
src/irmin-pack/unix/control_file.ml	`85.61% <40.00%> (-1.71%)`	⬇️
src/irmin-pack/unix/sparse_file.ml	`84.49% <93.54%> (+4.16%)`	⬆️
src/irmin-pack/unix/control_file_intf.ml	`82.14% <100.00%> (ø)`
src/irmin-pack/unix/file_manager.ml	`84.98% <100.00%> (+0.29%)`	⬆️
src/irmin-pack/unix/gc.ml	`73.40% <100.00%> (ø)`
src/irmin-pack/unix/lower.ml	`61.07% <100.00%> (+0.23%)`	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

metanivek · 2023-04-03T21:00:01Z

I still need to take a close look at the code, but as discussed earlier, I ran some preliminary benchmarks locally based on the benchmark I did last week. Early results are very promising! 😻 Below is a graph of average medians for two runs with LRU size of 0 (to focus on the mapping file perf) of no mmap (this PR) vs mmap (main, essentially). The title says "commit load times" but this is really "sample commits and load their entire trees"; it doesn't take this long to just "load a commit" 😄.

Based on these results, I definitely think it is worth doing a full replay trace benchmark (and potentially looking at the other optimization ideas you have!).

adatario · 2023-04-04T10:42:04Z

Results of a benchmark run using a Tezos Context replay: https://github.com/tarides/irmin-tezos-benchmarking/blob/main/benchmarks/2023-04-no-mmap/stats.txt

metanivek

Left some opening comments. I'm really excited about these changes -- thanks for working on it!

src/irmin-pack/unix/control_file_intf.ml

src/irmin-pack/unix/gc_worker.ml

metanivek · 2023-04-04T18:51:22Z

src/irmin-pack/unix/gc.mli

@@ -54,7 +54,7 @@ module Make (Args : Gc_args.S) : sig

  val cancel : t -> bool

-  val finalise_without_swap : t -> (int63 * int63) Lwt.t
+  val finalise_without_swap : t -> (int63 * int63 * int63) Lwt.t


Two thoughts:

For a future improvement, it would be nice to model this return type explicitly instead of a triple of int63s.

As is, the docs need updating to say what the third element is.

I went with producing a gced control file status here directly since it has all the required values for the store to then create the snapshot... but let me know if you think it's worse!

I kinda like it!

My only point of consideration is thinking about where the "knowledge" of constructing the Gced status ought to live.

If we wanted to embrace the Gc module having the knowledge of how to construct a control file Gced status after Gc finishes, we could also change the file manager's swap function to accept the status instead of creating it itself. But that is definitely for a separate PR!

src/irmin-pack/unix/gc_worker.ml

src/irmin-pack/unix/sparse_file.ml

metanivek · 2023-04-04T20:41:46Z

src/irmin-pack/unix/sparse_file.ml

Since we don't really use any of the "features" of Bigarray after this PR, I got curious if there is any performance benefit to using them. I did some simple benches and it would appear that we may get some benefit to switching to just Array. Perhaps already in your list of optimization ideas, but putting out there for some food for thought.

# dune exec arrays/main.exe Sum 1000 integers [Array#fold] 6.452000s [Array#safe] 0.761000s [Array#unsafe] 0.531000s [BigArray#safe] 15.919000s [BigArray#unsafe] 14.647000s Sample 100 out of 1000 [Array#safe_sample] 0.421000s [Array#unsafe_sample] 0.371000s [BigArray#safe] 1.222000s [BigArray#unsafe] 0.992000s

_{Results copied from here}

I'm guessing your benchmark is stressing the OCaml GC with all the Int64 custom objects... I added a patch to the "userland mmap" to not use bigarrays/Int64 in case you want to compare performances :)

(In this latest patch, I'm not sure if the array array is optimal, we could switch back to a flat array, but I wanted to see if it would help memory usage)

I tried it originally with floats and had similar results. I changed it to Int64 to mirror our setup and see if there was any observable difference.

I'll run another bench with your latest to see if there is any difference!

art-w · 2023-04-05T13:48:05Z

Thanks a lot for your benchmarks and feedbacks! <3

@adatario > If you have some time, I think there's a small glitch with the memory usage graph as it's a bit flat during GC.. I'm guessing it's because the GC is running in a child process and doesn't get measured by smem (?)

adatario · 2023-04-11T09:39:15Z

@adatario > If you have some time, I think there's a small glitch with the memory usage graph as it's a bit flat during GC.. I'm guessing it's because the GC is running in a child process and doesn't get measured by smem (?)

Here's a fixed graph:

metanivek · 2023-04-12T19:54:09Z

@adatario @art-w what do you think explains the higher memory during GC (but not otherwise)?

I originally thought it might be because we are observing the memory that used to be mmap'd (and thus maybe hidden from smem?), but then I would expect the entire graph to be shifted up. 🤔

metanivek · 2023-04-12T20:16:00Z

@art-w here is an updated bench with the "no bigarray" version. For now, lets stick with bigarray (unless you want to try a 1-D array as well). 😎

art-w · 2023-04-13T15:39:57Z

Thanks a lot @adatario !!

what do you think explains the higher memory during GC (but not otherwise)?

Yeah sorry I should have mentioned that I expected the memory usage to grow slightly during GC following PR #2215 (the current PR is unrelated). Previously an array was used to represent the Live ranges of the mapping file during GC, and this old PR switched to a list (same quantity of elements, but about 2/3 larger in memory used). I'll push a tighter representation of the Live ranges soon :)

adatario · 2023-04-14T07:36:47Z

Yeah sorry I should have mentioned that I expected the memory usage to grow slightly during GC following PR #2215 (the current PR is unrelated). Previously an array was used to represent the Live ranges of the mapping file during GC, and this old PR switched to a list (same quantity of elements, but about 2/3 larger in memory used). I'll push a tighter representation of the Live ranges soon :)

Nice! I've started another benchmark run with the memory optimizing commit you pushed.

art-w · 2023-04-14T07:47:33Z

No need, I managed to run it! (but thanks!)

It's not yet as good as before hm:

metanivek

LGTM!

src/irmin-pack/unix/sparse_file.ml

@samoht

…min-pack, irmin-pack-tools, irmin-mirage, irmin-mirage-graphql, irmin-mirage-git, irmin-http, irmin-graphql, irmin-git, irmin-fs, irmin-containers, irmin-cli, irmin-chunk and irmin-bench (3.7.0) CHANGES: ### Added - **irmin** - Add `Conf.pp` and `Conf.equal` to print and compare configuration values (mirage/irmin#2227, @samoht) - Add a `clear` optional arguments to all function that adds a new commit: `Commit.v`, `set`, `set_tree`, `remove`, `test_and_set`, `test_and_set_tree`, `test_set_and_get`, `test_set_and_get_tree`, `merge`, `merge_tree` and `with_tree`. This new argument allows to control whether the tree caches are cleared up after objects are exported to disk during the commit. (mirage/irmin#2225, @samoht) - **irmin-pack** - Add configuration option, `lower_root`, to specify a path for archiving data during a GC. (mirage/irmin#2177, @metanivek) - Add `is_split_allowed` to check if a store allows split. (mirage/irmin#2175, @metanivek) - Add `add_volume` to allow creating new empty volume in lower layer. (mirage/irmin#2188, @metanivek) - Add a `behaviour` function to the GC to check wether the GC will archive or delete data. (mirage/irmin#2190, @Firobe) - Add a migration on `open_rw` to move the data to the `lower_root` if the configuration was enabled (mirage/irmin#2205, @art-w) ### Changed - **irmin** - Expose type equality for `Schema.Info` to avoid defining the `info` function multiple times when using similar stores (mirage/irmin#2189, mirage/irmin#2193, @samoht) - **irmin-pack** - GC now changes its behaviour depending on the presence of a lower layer. (mirage/irmin#2190, @Firobe) - Split now raises an exception if it is not allowed. It is not allowed on stores that do not allow GC. (mirage/irmin#2175, @metanivek) - GC now supports stores imported V1/V2 stores, in presence of a lower layer only. (mirage/irmin#2190, @art-w, @Firobe) - Upgrade on-disk format to version 5. (mirage/irmin#2184, @metanivek) - Archive to lower volume does not copy orphaned commits. (mirage/irmin#2215, @art-w) ### Fixed - **irmin-pack** - Unhandled exceptions in GC worker process are now reported as a failure (mirage/irmin#2163, @metanivek) - Fix the silent mode for the integrity checks. (mirage/irmin#2179, @icristescu) - Fix file descriptor leak caused by `mmap`. (mirage/irmin#2232, @art-w)

art-w requested a review from metanivek March 30, 2023 15:38

metanivek reviewed Apr 4, 2023

View reviewed changes

art-w force-pushed the no-mmap branch 2 times, most recently from 1616977 to 3728b13 Compare April 14, 2023 00:22

art-w force-pushed the no-mmap branch from 3728b13 to 1e8a99e Compare April 14, 2023 15:26

metanivek approved these changes Apr 17, 2023

View reviewed changes

src/irmin-pack/unix/sparse_file.ml Outdated Show resolved Hide resolved

art-w added 2 commits April 18, 2023 18:09

irmin-pack: userland mmap

794b7f9

irmin-pack: remember expected mapping size in upper control file

ccaa77e

art-w force-pushed the no-mmap branch from 1e8a99e to ccaa77e Compare April 18, 2023 16:09

metanivek merged commit 03ad27f into mirage:main Apr 19, 2023

This was referenced Apr 21, 2023

irmin-pack: extend GC to unlimited history stores #2182

Closed

[new release] Irmin project (3.7.0) ocaml/opam-repository#23698

Merged

metanivek added tezos-support Support for bugs related to Tezos and removed tezos-support Support for bugs related to Tezos labels Apr 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

irmin-pack: no mmap syscall #2232

irmin-pack: no mmap syscall #2232

art-w commented Mar 30, 2023

codecov-commenter commented Mar 30, 2023 •

edited

Loading

metanivek commented Apr 3, 2023 •

edited

Loading

adatario commented Apr 4, 2023

metanivek left a comment

metanivek Apr 4, 2023

art-w Apr 5, 2023

metanivek Apr 10, 2023

metanivek Apr 4, 2023

art-w Apr 5, 2023

metanivek Apr 10, 2023

art-w commented Apr 5, 2023

adatario commented Apr 11, 2023

metanivek commented Apr 12, 2023 •

edited

Loading

metanivek commented Apr 12, 2023 •

edited

Loading

art-w commented Apr 13, 2023

adatario commented Apr 14, 2023

art-w commented Apr 14, 2023

metanivek left a comment

irmin-pack: no mmap syscall #2232

irmin-pack: no mmap syscall #2232

Conversation

art-w commented Mar 30, 2023

codecov-commenter commented Mar 30, 2023 • edited Loading

Codecov Report

metanivek commented Apr 3, 2023 • edited Loading

adatario commented Apr 4, 2023

metanivek left a comment

Choose a reason for hiding this comment

metanivek Apr 4, 2023

Choose a reason for hiding this comment

art-w Apr 5, 2023

Choose a reason for hiding this comment

metanivek Apr 10, 2023

Choose a reason for hiding this comment

metanivek Apr 4, 2023

Choose a reason for hiding this comment

art-w Apr 5, 2023

Choose a reason for hiding this comment

metanivek Apr 10, 2023

Choose a reason for hiding this comment

art-w commented Apr 5, 2023

adatario commented Apr 11, 2023

metanivek commented Apr 12, 2023 • edited Loading

metanivek commented Apr 12, 2023 • edited Loading

art-w commented Apr 13, 2023

adatario commented Apr 14, 2023

art-w commented Apr 14, 2023

metanivek left a comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 30, 2023 •

edited

Loading

metanivek commented Apr 3, 2023 •

edited

Loading

metanivek commented Apr 12, 2023 •

edited

Loading

metanivek commented Apr 12, 2023 •

edited

Loading