irmin-pack: remove append-only external flush callback #2235
Conversation
art-w
force-pushed
the
ao-dict
branch
2 times, most recently
from
22cf7f8
to
587af95
Compare
| let* () = flush_suffix t in | ||
| let+ () = flush_control t in | ||
| List.iter (fun { after_flush } -> after_flush ()) t.suffix_consumers |
There was a problem hiding this comment.
The previous code skipped writing the control file and running after_flush if the suffix was empty. I like the straightforward nature of your refactor, so I don't think we should necessarily maintain previous behavior (and I don't think it matters in practice). I mostly wanted to point it out in case you have thoughts about it.
There was a problem hiding this comment.
Thanks :) I hope the control file rewrite is avoided by line 149 above! (if new_pl = pl then Ok ())
I believe the only suffix consumer is used to clear the staging table in Pack_store so it should have little impact... and I don't think this staging reset should be setup like that anyway, it's a bit obfuscated :P
| @@ -21,23 +21,19 @@ module Make (Io : Io.S) (Errs : Io_errors.S with module Io = Io) = struct | |||
| module Io = Io | |||
| module Errs = Errs | |||
|
|
|||
| let auto_flush_threshold = 16_384 | |||
There was a problem hiding this comment.
I'm running a benchmark to see what impact this has.
There was a problem hiding this comment.
Ran a benchmark that was slower but then I remembered that 3.8 changed LRU semantics. I need to re-run with an adjusted LRU size.
There was a problem hiding this comment.
I ran the benchmark with adjusted LRU entry size. https://github.com/metanivek/irmin-tezos-benchmarking/tree/rm_auto_flush/benchmarks/2023-07-pr-2235
I'm having issues with generating stats.txt for the benchmark (separate issue) but here are the results.
The first comparison is against 3.7.1 baseline that we've been using.
This makes this PR look like it is a huge performance improvement. Well, then I remembered that the new LRU performs better when compared to 3.7.
So here is this PR compared to the equivalent run for the new LRU (15k entries).
I don't know the exact numbers since I can't get the summary data yet, but I can't imagine it is much different, given how closely these lines are. I think this PR is good to go from a performance perspective!
There was a problem hiding this comment.
Thanks a ton for running the benchmarks! It's great to have confirmation that this PR doesn't have any impact as expected :)
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #2235 +/- ##
==========================================
- Coverage 68.17% 68.17% -0.01%
==========================================
Files 138 138
Lines 16739 16733 -6
==========================================
- Hits 11412 11407 -5
+ Misses 5327 5326 -1
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I was trying to simplify the logic to update the control file in the file manager. The
Append_only_filewas using an external callback to signify the need to flush when its buffer was full, which would in turn update the control file (to make that portion of the file visible)... but then we could end up with a visible partially written commit, which isn't an issue in itself, but isn't how we think about the control file payload. As the flush threshold was fairly high, I don't think this "partial flush" was really stress tested either (?)Anyway it's gone, as it should be fine to just write to the append-only files when their memory buffer is full, we only need to update the control file at the end to publish the writes.
While testing I discovered that I broke the
Dictflushing itsend_poffto the control file, so I refactored that a bit to match with the chunks/sparse file abstractions (inverting theFile_managerdependency). A few breaking changes here: