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

Tree.remove scaling #1289

Merged
merged 8 commits into from
Feb 15, 2021
Merged

Tree.remove scaling #1289

merged 8 commits into from
Feb 15, 2021

Conversation

Ngoguey42
Copy link
Contributor

@Ngoguey42 Ngoguey42 commented Feb 10, 2021

Resolves #1272

New perfs

Commit it now ~14 times quicker, it is repaired.

Remove is now only ~3 times quicker because of the poor implementation of Tree.Node.is_empty.

Master - flatten

dune exec -- ./bench/irmin-pack/tree.exe --mode trace --inode-config 32 --flatten  ~/boostrap_trace.json
Replaying trace  13300/13315 commits  09:16  [##########################.]  99%
Tezos_log mode on inode config [32, 256]: 13315 commits
895827 "Add" 2.376 sec (0.4%)
54196 "Remove" 204.781 sec (37.1%)
2950152 "Find" 12.638 sec (2.3%)
669692 "Mem" 2.185 sec (0.4%)
439329 "Mem_tree" 0.528 sec (0.1%)
13314 "Checkout" 0.029 sec (0.0%)
59 "Copy" 0.000 sec (0.0%)
13315 "Commit" 329.685 sec (59.7%)
Total time: 551.869637
Size on disk: 114 M

13k_flatten_master

This PR - flatten

dune exec -- ./bench/irmin-pack/tree.exe --mode trace --inode-config 32 --flatten  ~/boostrap_trace.json
Replaying trace  13300/13315 commits  02:45  [##########################.]  99%
Tezos_log mode on inode config [32, 256]: 13315 commits
895827 "Add" 1.966 sec (1.9%)
54196 "Remove" 67.908 sec (66.5%)
2950152 "Find" 7.454 sec (7.3%)
669692 "Mem" 1.158 sec (1.1%)
439329 "Mem_tree" 0.375 sec (0.4%)
13314 "Checkout" 0.022 sec (0.0%)
59 "Copy" 0.000 sec (0.0%)
13315 "Commit" 23.207 sec (22.7%)
Total time: 104.945730
Size on disk: 114 M

13k_flatten_rmscale

@Ngoguey42 Ngoguey42 changed the title Tree remove scaling Tree.remove scaling Feb 11, 2021
Copy link
Member

@craigfe craigfe left a comment

Choose a reason for hiding this comment

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

Nice improvement; thanks.

src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
src/irmin/tree.ml Outdated Show resolved Hide resolved
Copy link
Member

@craigfe craigfe left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @Ngoguey42. Merge when ready and I'll cut a release.

@Ngoguey42
Copy link
Contributor Author

Thanks for the review

@Ngoguey42 Ngoguey42 merged commit 7c80af4 into mirage:master Feb 15, 2021
craigfe added a commit to craigfe/opam-repository that referenced this pull request Feb 16, 2021
… irmin-chunk, irmin-pack, irmin-test, irmin-http, irmin-unix, ppx_irmin, irmin-bench, irmin-graphql, irmin-containers, irmin-mirage-git and irmin-mirage-graphql (2.5.0)

CHANGES:

### Changed

- **irmin**
  - `Store.Tree.remove` is now much faster when operating on large directories.
    The commits following removals are also much faster. (mirage/irmin#1289, @Ngoguey42)

  - Changed `Store.Tree.{of_hash, shallow}` to take kinded hashes, allowing the
    creation of unforced contents values. (mirage/irmin#1285, @craigfe)

  - Changed `Tree.destruct` to return _lazy_ contents values, which may be forced
    with `Tree.Contents.force`. (mirage/irmin#1285, @craigfe)

- **irmin-bench**
  - New features in benchmarks for tree operations (mirage/irmin#1269, @Ngoguey42)
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.

Inode.v is inefficient
2 participants