Use accessor in pack store #2070
Conversation
Ngoguey42
force-pushed
the
use-accessor-in-pack_store
branch
3 times, most recently
from
ba9bc54 to
b7d723e
Compare
|
It's ready for review. It however doesn't pass #2068 |
src/irmin-pack/unix/pack_store.ml
Outdated
| || Lru.mem t.lru (offset_of_key t k) | ||
| || pack_file_contains_key t k | ||
| match Pack_key.inspect k with | ||
| | Indexed hash -> Tbl.mem t.staging hash || pack_file_contains_key t k |
There was a problem hiding this comment.
this is a bit confusing for me: we can have in staging indexed keys, but not in the lru?
There was a problem hiding this comment.
The key doesn't contain an offset, so its overall easier to directly lookup on disk
| | Indexed hash -> ( | ||
| match Tbl.find t.staging hash with | ||
| | v -> | ||
| (* Hit in staging, but we don't have offset to put in LRU *) |
There was a problem hiding this comment.
This is changing a bit the behaviour wrt the lru, right?
There was a problem hiding this comment.
Yep. I think it makes sense beceause we've migrated from a hash-keyed lru to an offset-keyed lru
There was a problem hiding this comment.
its still not very clear to me: whenever we add a hash in staging we also add an offset in the lru.
with this change I think now we can promote values indexed keys in the lru (via Lru.add and Lru.mem in unsafe_mem) only once they left the staging? (it probably does not change much in practice, as its unlikely to have indexed keys that have a hash in staging)
There was a problem hiding this comment.
I added an assert false for this case (indexed hash in staging) and the test suite passes
There was a problem hiding this comment.
Let's postpone this discussion for now, and we can come back to it later (opened #2077).
Ngoguey42
force-pushed
the
use-accessor-in-pack_store
branch
from
b7d723e to
c1cd0d7
Compare
|
Thanks for the review @icristescu |
|
The CB reports +0.2% objects allocated. I think it's safe to extrapolate this result to the 5h-long benchmark. @icristescu what do you think? |
Yes, let's not block for this. |
This commit is meant to better handle objects that are still live in index but that have been GCed.
Ngoguey42
force-pushed
the
use-accessor-in-pack_store
branch
from
c1cd0d7 to
f43749f
Compare
|
Merge when green |
…min-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.4.1) CHANGES: ### Added - **irmin** - Add `Storage` module for creating custom storage layers (mirage/irmin#2047, @metanivek) - **irmin-pack** - Add `Gc.is_allowed` (mirage/irmin#2076, @icristescu) - Add a `weight` parameter in the LRU implementation to bound memory usage (mirage/irmin#2050, @samoht) ### Changed - **irmin** - Removed `Irmin_unix.set_listen_dir_hook` (mirage/irmin#2071, @zshipko) ### Fixed - **irmin-pack** - Fix the behaviour of irmin-pack regarding hashes and keys to GCed objects. It used to not correctly ignore these entries, which could have resulted in various bugs. E.g. the impossibility to append an object that used to be dead and that has its hash in index. (mirage/irmin#2070, @Ngoguey42)
irmaTS
added
tezos-support
No description provided.