Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gc stats #2089

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Gc stats #2089

merged 2 commits into from
Oct 6, 2022

Conversation

Ngoguey42
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2022

Codecov Report

Merging #2089 (01c43cb) into main (4ab886c) will decrease coverage by 0.19%.
The diff coverage is 51.55%.

@@            Coverage Diff             @@
##             main    #2089      +/-   ##
==========================================
- Coverage   64.69%   64.49%   -0.20%     
==========================================
  Files         132      131       -1     
  Lines       15719    15828     +109     
==========================================
+ Hits        10169    10208      +39     
- Misses       5550     5620      +70     
Impacted Files Coverage Δ
bench/irmin-pack/trace_replay.ml 41.46% <0.00%> (-0.17%) ⬇️
src/irmin-pack/unix/ext.ml 61.78% <ø> (+0.09%) ⬆️
src/irmin-pack/unix/stats.ml 50.58% <21.05%> (-7.63%) ⬇️
src/irmin-pack/unix/gc.ml 39.22% <38.33%> (-5.67%) ⬇️
src/irmin-pack/unix/dispatcher.ml 52.80% <41.66%> (+1.21%) ⬆️
src/irmin-pack/unix/control_file.ml 86.48% <66.66%> (+1.12%) ⬆️
src/irmin-pack/unix/io.ml 65.76% <70.58%> (+0.87%) ⬆️
src/irmin-pack/unix/mapping_file.ml 93.75% <80.00%> (-0.50%) ⬇️
src/irmin-pack/unix/append_only_file.ml 85.00% <85.71%> (ø)
src/irmin-pack/unix/stats_intf.ml 85.00% <91.66%> (+35.00%) ⬆️
... and 10 more

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

@Ngoguey42
Copy link
Contributor Author

Ngoguey42 commented Sep 22, 2022

gc.ml is about 900 lines of code. I'd like to split it into Gc_utils (which would contain the new stats), Gc_worker and Gc at some point (not in this PR, after this PR is merged). @metanivek you did not fancy that solution last time we talked about this. WDYT now?

@metanivek
Copy link
Member

metanivek commented Sep 22, 2022

I usually think that "utils" are a code smell. Sometimes they are warranted as a collection of utility functions, but I usually want to pause and think about what I'm actually trying to model by stuffing things into a "utils". I don't know if this translates well, but I see them as the junk drawer of code bases. 🗑️

As for gc specifically, I shied away from it previously because of the above utils issue, but also I didn't see a strong need to pull apart the module since it all works as one unit anyway (the almost identical Args module indicating this strongly to me). I'm also noticing that lots of OCaml code bases have large module files (sometimes in the thousands of lines), and I wonder if it is just more common to place more code into files with OCaml than other languages.

All that to say, I wouldn't be opposed to splitting the file, but I would just like to see a better modelling than a utils file. 😉

@Ngoguey42 Ngoguey42 force-pushed the gc-stats branch 2 times, most recently from 9b1f1bb to dc4c3ce Compare September 27, 2022 13:49
@Ngoguey42 Ngoguey42 changed the title (WIP) Gc stats Gc stats Sep 27, 2022
@Ngoguey42
Copy link
Contributor Author

This is now ready for review.

@icristescu icristescu mentioned this pull request Sep 27, 2022
34 tasks
Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a high level review pass (not looking closely at the details of the stats calculations). I really (really) think we ought to separate these stats from the ones surfaced in the public API. Left a few other comments, but this my only real change request for this PR. Thanks for doing the work to get these stats in place!

examples/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/stats_intf.ml Show resolved Hide resolved
src/irmin-pack/unix/mapping_file_intf.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/mapping_file.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/io_errors.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/io.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/io_errors.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/stats_intf.ml Show resolved Hide resolved
src/irmin-pack/unix/io_errors.ml Outdated Show resolved Hide resolved
examples/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/stats_intf.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Outdated Show resolved Hide resolved
src/irmin-pack/unix/gc.ml Show resolved Hide resolved
src/irmin-pack/unix/stats_intf.ml Show resolved Hide resolved
@Ngoguey42
Copy link
Contributor Author

I'm done with this PR. I'll clean the commit history prior to merge, following approves

Copy link
Member

@metanivek metanivek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small change requested and then you can !fix!up!!

src/irmin-pack/unix/stats_intf.ml Show resolved Hide resolved
@Ngoguey42 Ngoguey42 merged commit 396e01c into mirage:main Oct 6, 2022
metanivek added a commit to metanivek/opam-repository that referenced this pull request Dec 14, 2022
…ils, irmin-test, irmin-pack, 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.5.0)

CHANGES:

### Added

- **irmin-pack**
  - Add `Irmin_pack_unix.Stats.Latest_gc` which is now the parameter of GC's
    `finished` callback (mirage/irmin#2089, @Ngoguey42)
  - Add `Gc.oldest_live_commit` which returns the key of the commit on which the
    latest gc was called on. (mirage/irmin#2110, @icristescu)
  - Add `split` to create a new suffix chunk. Subsequent writes will append to
    this chunk until `split` is called again. (mirage/irmin#2118, @icristescu)
  - Add `create_one_commit_store` to create a new store from the existing one,
    containing only one commit. (mirage/irmin#2125, @icristescu)

### Changed

- **irmin-pack**
  - Upgraded on-disk format to version 4. (mirage/irmin#2110, @icristescu)
  - Detecting control file corruption with a checksum (mirage/irmin#2119, @art-w)
  - Change on-disk layout of the suffix from a single file to a multiple,
    chunked file design (mirage/irmin#2115, @metanivek)
  - Modify GC to work with new chunked suffix. See `examples/gc.ml` for a
    demonstration of how it works with the new `split` function. (mirage/irmin#2126,
    @metanivek)

### Fixed
metanivek added a commit to metanivek/opam-repository that referenced this pull request Dec 14, 2022
…ils, irmin-test, irmin-pack, 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.5.0)

CHANGES:

### Added

- **irmin-pack**
  - Add `Irmin_pack_unix.Stats.Latest_gc` which is now the parameter of GC's
    `finished` callback (mirage/irmin#2089, @Ngoguey42)
  - Add `Gc.oldest_live_commit` which returns the key of the commit on which the
    latest gc was called on. (mirage/irmin#2110, @icristescu)
  - Add `split` to create a new suffix chunk. Subsequent writes will append to
    this chunk until `split` is called again. (mirage/irmin#2118, @icristescu)
  - Add `create_one_commit_store` to create a new store from the existing one,
    containing only one commit. (mirage/irmin#2125, @icristescu)

### Changed

- **irmin-pack**
  - Upgraded on-disk format to version 4. (mirage/irmin#2110, @icristescu)
  - Detecting control file corruption with a checksum (mirage/irmin#2119, @art-w)
  - Change on-disk layout of the suffix from a single file to a multiple,
    chunked file design (mirage/irmin#2115, @metanivek)
  - Modify GC to work with new chunked suffix. See `examples/gc.ml` for a
    demonstration of how it works with the new `split` function. (mirage/irmin#2126,
    @metanivek)

### Fixed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants