Optimizes Cargo's registry cache format for fewer files #6908
Comments
ehuss
added
the
A-caching
|
A easier way to see how big a difference this makes may be to do a single file cache using sqlite or sled. Then when we know if it works we can have the discussion of if the dependency is worth it, or if we should implement our own format. |
minimal-copy `deserialize` for `InternedString` I just learnt that `serde::Deserialize` for `Cow<'a, str>` allocates by default! Thus negating the intended benefit of ea957da, and this is in the hot loop for no-op builds #6908. The docs https://serde.rs/lifetimes.html#borrowing-data-in-a-derived-impl say you can fix this with a `#[serde(borrow)]`, but in practice this does not work on `Option<Cow<'a, str>>`. Some of these are just going to be turned into `InternedString`s, so we can tell serde to do that directly saving an allocation while we are at it! So is this faster, or just reducing the number of `InternedString` <-> `&str` conversions? I ran the benchmark script developed for #7168 (comment). Looks like no change for Cargo's lockfile and a ~7% improvement for the 2000 crate stress test.
|
I've beem experimenting some naive implementation for SQLite and sled but the result seems not better than the current in-house cache mechanism. Both of the implementations store almost the same format, "package name -> content blob" key-value pairs, inside databases. The only slight difference is that I name the database files Though the benchmark result seems not that great, it probably has some room to improve from SQLite side. For sled I have no idea how to tune it. I would love to do more investigation if needed. Any thought or direction are welcome! Environment information:
The patches are listed as below:
The benchmark are run against cargo project itself by hyperfine.
Raw benchmark data from hyperfine |
|
Thank you so much for doing this work! |
|
Sorry about missing hardware information. Comment updated. 😅 |
Benchmark results for large projectstl;dr, no significant performance improvement for these three implementations. The SQLite version is almost at the same speed comparing to the in-house cache mechanism, whereas sled version is slightly slow but is negligible to me. Environment information:
The patches:
Space usage in
Expand to see benchmark data#!/usr/bin/env bash
pkgs=$(ls -d */)
benchmark_at="$(date +%s)"
for pkg in ${pkgs[@]}; do
pkg="${pkg%/}"
echo "${pkg}"
pushd "${pkg}"
offline="--offline";
# diem and substrate have deps conflicts with `--offline`
if [[ "$pkg" -eq "diem" ]] || [[ "$pkg" -eq "substrate" ]]; then
offline=
fi
fn="${pkg}-${benchmark_at}-result"
hyperfine -L offline "${offline}" -w 2 \
'cargo/target/release/cargo generate-lockfile -Zno-index-update {offline}' -n master \
'cargo+cache-sled/target/release/cargo generate-lockfile -Zno-index-update {offline}' -n sled \
'cargo+cache-sqlite/target/release/cargo generate-lockfile -Zno-index-update {offline}' -n sqlite \
--export-markdown "../${fn}.md"
popd
donediem/diem@05bdd16
mozilla/gecko-dev@5977b6f
rust-lang/rust@59579907
servo/servo@d1673446
paritytech/substrate@be1b8ef0
tikv/tikv@06c3e76e
|
|
I made some changes for SQLite according to suggestions on zulip:
Still, no significant difference between the two.
benchmark details#!/usr/bin/env bash
pkgs=$(/bin/ls -d */)
benchmark_at="$(date +%s)"
for pkg in ${pkgs[@]}; do
pkg="${pkg%/}"
echo "${pkg}"
pushd "${pkg}"
offline="--offline";
# diem and substrate have deps conflicts with `--offline`
if [[ "$pkg" -eq "diem" ]] || [[ "$pkg" -eq "substrate" ]]; then
offline=
fi
fn="${pkg}-${benchmark_at}-result"
hyperfine -L offline "${offline}" -w 2 \
-p 'mkdir -p ~/.cargo/registry/index/git.luolix.top-1ecc6299db9ec823/.cache' \
-c 'rm -rf ~/.cargo/registry/index/git.luolix.top-1ecc6299db9ec823/.cache' \
'sqlite/target/release/cargo generate-lockfile -Zno-index-update {offline}' -n sqlite \
'sqlite-wal/target/release/cargo generate-lockfile -Zno-index-update {offline}' -n sqlite-wal \
--export-markdown "../${fn}.md"
popd
donediem/diem@05bdd16
mozilla/gecko-dev@5977b6f
rust-lang/rust@5957990
servo/servo@d167344
paritytech/substrate@be1b8ef
tikv/tikv@06c3e76
|
Benchmark on WindowsDone some benchmark on Windows but the machine lacks lots of dependencies, so only a portion of test cases are included. We can see that SQLite outperforms original per-file version by 1.27-1.40x, and faster than seld from 1.04-1.60x. Great result so far! expand the benchmark detailsEnvironment information:
b1684e2 (cargo)
paritytech/substrate@4652f9e
tikv/tikv@eee35b7
|
|
#12634 is adding sqlite to cargo. With the improvements this made to Windows, @weihanglo should we move this forward? Would a |
|
This is include in the plan: https://hackmd.io/U_k79wk7SkCQ8_dJgIXwJg. And yes this is something likely can be done after #12634. I am assuming @ehuss already got something. |
weihanglo
added
the
S-needs-mentor
|
#13584 is another attempt to integrate SQLite for index cache. It basically replicates the behavior of the current filesystem cache, which stores entire JSON blobs from the index. Not ideal and there are performance losses in cache writes. On the official SQLite website they have benchmarked external blobs (Cargo's current behvior) and blobs-all-in-SQLite. Unsurprisingly on Linux it is performant to manipulate file IO. I haven't done any benchmark on Windows, as I have no machine at this moment, though I know Windows is the area that will potentially gain a lot benefits from this. However, given cache writes has a poor performance with SQLite. I wonder if the experiment worth putting more efforts. Something we could do
|
First implemented in #6880 Cargo now has an on-disk cache for the registry index which avoids loading from git and parsing extraneous JSON which doesn't end up getting used.
This format isn't fantastic for Windows, however, since it has an extra file-per-crate in the index. @Eh2406 recommended a different implementation strategy which would reduce the number of files in play, and we should likely implement that!
Implementation history:
std::fsaway from on-disk index cache #13515The text was updated successfully, but these errors were encountered: