[fix] Make Tree roundtrip by storing additional bit of information #1917
Conversation
pierrechevalier83
force-pushed
the
issue_1887
branch
5 times, most recently
from
a60bdd6 to
27149ee
Compare
There was a problem hiding this comment.
Thanks a lot for tackling this! It's much appreciated and I hope we can sort this out together.
First of all, the commits would need to be refactored so that fix!: or change!: prefixes are used only on the changes that are breaking, on the crate that is breaking (probabkly gix-object), followed by a commit that says
adapt to changes in gix-object to fix breakage. That way cargo smart-release will know what's happening and deal with changelogs correctly. The example given here is for when only gix-object has the initial breaking change.
Secondly, and this is the reason this PR needs changes, unconditionally allocating for EntryMode seems unacceptable performance wise (see below), and I hope it's unnecessary overall if [u8;6] or similar (maybe as backing for SmallVec) is used instead. My preference is to limit the amount of mode-bytes that can be stored to get EntryMode to be copy, removing the need for EntryModeRef entirely.
Performance Results
On main
At the worst, it's 45% slower for dealing with trees in the TreeRefIter() benchmark. Performance there is critical to being fast when diffing, and overall tree performance is relevant for many, many operations. The EntryMode can't be slowing things down this much, even though I'd not be surprised if it slows a percent or two if [u8; 6 (or 8)] is used as backing.
gitoxide/gix-object ( main) [$?]
❯ cargo bench
Compiling gix-hash v0.16.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-hash)
Compiling gix-features v0.40.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-features)
Compiling gix-hashtable v0.7.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-hashtable)
Compiling gix-object v0.47.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-object)
Compiling gix-fs v0.13.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-fs)
Compiling gix-pack v0.57.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-pack)
Compiling gix-odb v0.67.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-odb)
Finished `bench` profile [optimized] target(s) in 11.51s
Running unittests src/lib.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/gix_object-2e38e5f5f374a67d)
running 9 tests
test commit::message::body::test_parse_trailer::extra_whitespace_before_token_or_value_is_error ... ignored
test commit::message::body::test_parse_trailer::simple_newline ... ignored
test commit::message::body::test_parse_trailer::simple_newline_windows ... ignored
test commit::message::body::test_parse_trailer::simple_non_ascii_no_newline ... ignored
test commit::message::body::test_parse_trailer::with_lots_of_whitespace_newline ... ignored
test data::tests::size_of_object ... ignored
test tag::write::tests::validated_name::invalid::leading_dash ... ignored
test tag::write::tests::validated_name::invalid::only_dash ... ignored
test tag::write::tests::validated_name::valid::version ... ignored
test result: ok. 0 passed; 0 failed; 9 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running benches/decode_objects.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/decode_objects-5e550e088f5c4ce3)
Gnuplot not found, using plotters backend
CommitRef(sig) time: [904.52 ns 909.42 ns 915.09 ns]
change: [+0.1128% +0.5391% +0.9686%] (p = 0.01 < 0.05)
Change within noise threshold.
Found 7 outliers among 100 measurements (7.00%)
4 (4.00%) high mild
3 (3.00%) high severe
CommitRefIter(sig) time: [1.0055 µs 1.0074 µs 1.0095 µs]
change: [-0.0876% +0.1536% +0.3638%] (p = 0.21 > 0.05)
No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
3 (3.00%) high mild
3 (3.00%) high severe
TagRef(sig) time: [211.26 ns 212.49 ns 213.65 ns]
change: [+4.1348% +4.4867% +4.8600%] (p = 0.00 < 0.05)
Performance has regressed.
Found 20 outliers among 100 measurements (20.00%)
2 (2.00%) low mild
9 (9.00%) high mild
9 (9.00%) high severe
TagRefIter(sig) time: [196.21 ns 196.33 ns 196.47 ns]
Found 18 outliers among 100 measurements (18.00%)
8 (8.00%) high mild
10 (10.00%) high severe
TreeRef() time: [111.29 ns 111.69 ns 112.15 ns]
TreeRefIter() time: [45.203 ns 45.228 ns 45.260 ns]
Found 17 outliers among 100 measurements (17.00%)
17 (17.00%) high severe
Running benches/edit_tree.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/edit_tree-2ac917c6b895fb51)
Gnuplot not found, using plotters backend
editor/small tree (empty -> full -> empty)
time: [2.3269 µs 2.3296 µs 2.3331 µs]
thrpt: [4.2861 Melem/s 4.2927 Melem/s 4.2975 Melem/s]
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe
editor/deeply nested tree (empty -> full -> empty)
time: [7.3327 µs 7.3355 µs 7.3385 µs]
thrpt: [6.2683 Melem/s 6.2709 Melem/s 6.2732 Melem/s]
Found 7 outliers among 100 measurements (7.00%)
3 (3.00%) high mild
4 (4.00%) high severe
cursor/small tree (empty -> full -> empty)
time: [2.4223 µs 2.4239 µs 2.4255 µs]
thrpt: [4.1229 Melem/s 4.1256 Melem/s 4.1282 Melem/s]
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild
cursor/deeply nested tree (empty -> full -> empty)
time: [2.6659 µs 2.6690 µs 2.6725 µs]
thrpt: [17.212 Melem/s 17.235 Melem/s 17.255 Melem/s]
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe
gitoxide/gix-object ( main) [$?] took 1m46s
On this PR
gitoxide/gix-object ( issue_1887) [$?] took 5s
❯ cargo bench
Compiling gix-object v0.47.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-object)
Compiling gix-pack v0.57.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-pack)
Compiling gix-odb v0.67.0 (/Users/byron/dev/github.com/Byron/gitoxide/gix-odb)
Finished `bench` profile [optimized] target(s) in 11.47s
Running unittests src/lib.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/gix_object-2e38e5f5f374a67d)
running 9 tests
test commit::message::body::test_parse_trailer::extra_whitespace_before_token_or_value_is_error ... ignored
test commit::message::body::test_parse_trailer::simple_newline ... ignored
test commit::message::body::test_parse_trailer::simple_newline_windows ... ignored
test commit::message::body::test_parse_trailer::simple_non_ascii_no_newline ... ignored
test commit::message::body::test_parse_trailer::with_lots_of_whitespace_newline ... ignored
test data::tests::size_of_object ... ignored
test tag::write::tests::validated_name::invalid::leading_dash ... ignored
test tag::write::tests::validated_name::invalid::only_dash ... ignored
test tag::write::tests::validated_name::valid::version ... ignored
test result: ok. 0 passed; 0 failed; 9 ignored; 0 measured; 0 filtered out; finished in 0.00s
Running benches/decode_objects.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/decode_objects-5e550e088f5c4ce3)
Gnuplot not found, using plotters backend
CommitRef(sig) time: [902.61 ns 904.94 ns 907.23 ns]
change: [-0.5721% -0.0929% +0.3624%] (p = 0.70 > 0.05)
No change in performance detected.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe
CommitRefIter(sig) time: [1.0197 µs 1.0234 µs 1.0271 µs]
change: [+0.5597% +0.8922% +1.2066%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild
TagRef(sig) time: [204.63 ns 205.08 ns 205.61 ns]
change: [-2.3882% -1.9912% -1.6128%] (p = 0.00 < 0.05)
Performance has improved.
TagRefIter(sig) time: [197.77 ns 197.98 ns 198.20 ns]
change: [+0.7578% +0.9111% +1.0753%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
5 (5.00%) high mild
1 (1.00%) high severe
TreeRef() time: [134.85 ns 135.15 ns 135.45 ns]
change: [+20.248% +20.620% +20.990%] (p = 0.00 < 0.05)
Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
5 (5.00%) high mild
1 (1.00%) high severe
TreeRefIter() time: [65.638 ns 65.705 ns 65.772 ns]
change: [+45.141% +45.347% +45.585%] (p = 0.00 < 0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
1 (1.00%) high mild
2 (2.00%) high severe
Running benches/edit_tree.rs (/Users/byron/dev/github.com/Byron/gitoxide/target/release/deps/edit_tree-2ac917c6b895fb51)
Gnuplot not found, using plotters backend
editor/small tree (empty -> full -> empty)
time: [2.5407 µs 2.5503 µs 2.5625 µs]
thrpt: [3.9025 Melem/s 3.9211 Melem/s 3.9360 Melem/s]
change:
time: [+8.7291% +9.0199% +9.3622%] (p = 0.00 < 0.05)
thrpt: [-8.5607% -8.2736% -8.0283%]
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe
editor/deeply nested tree (empty -> full -> empty)
time: [8.0135 µs 8.0283 µs 8.0419 µs]
thrpt: [5.7200 Melem/s 5.7298 Melem/s 5.7403 Melem/s]
change:
time: [+8.9428% +9.1939% +9.4636%] (p = 0.00 < 0.05)
thrpt: [-8.6455% -8.4198% -8.2087%]
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe
cursor/small tree (empty -> full -> empty)
time: [2.6577 µs 2.6610 µs 2.6641 µs]
thrpt: [3.7536 Melem/s 3.7580 Melem/s 3.7627 Melem/s]
change:
time: [+9.6125% +9.7602% +9.9436%] (p = 0.00 < 0.05)
thrpt: [-9.0443% -8.8923% -8.7695%]
Performance has regressed.
Found 10 outliers among 100 measurements (10.00%)
7 (7.00%) low mild
2 (2.00%) high mild
1 (1.00%) high severe
cursor/deeply nested tree (empty -> full -> empty)
time: [2.9118 µs 2.9173 µs 2.9227 µs]
thrpt: [15.739 Melem/s 15.768 Melem/s 15.798 Melem/s]
change:
time: [+8.9949% +9.2424% +9.4751%] (p = 0.00 < 0.05)
thrpt: [-8.6551% -8.4604% -8.2526%]
Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severe
gitoxide/gix-object ( issue_1887) [$?] took 1m47s
Yes, that would work and actually will make the change much less intrusive, which is nice. Not I'll also rename the commits so that the history works with |
|
Thank you! [> Yes, that would work and actually will make the change much less intrusive, which is nice. Not It shouldn't have, from all I know is that there is a couple of mode-bytes and we want to keep them verbatim. If we would have a copyable storage for enough bytes, we could keep the mode bytes verbatim and convert to other forms/decode it on the fly. |
|
Uhm. I guess |
|
Great, I am really looking forward to seeing what that does to the complexity of the implementation, and to the performance. |
I've done the portion of the changes limited to For me,
I'm now thinking that locality of the I can suggest a solution. It may be hacky, but it should work: Observations:
Suggestion:
This will lead to slightly tricky code in the Does that sound like a good trade-off, given the performance impact we've measured? |
|
Thanks again for tackling this and for validating the new implementation by running the benchmarks. I wouldn't have thought that Besides that, doing so might be the lead invasive solution, so I'd also think that it should be tried. Thanks again. |
OK. I'll try that next and see how it affects performance. |
|
Actually, on Do we have a good benchmark that's more representative of a real workload (like computing blame on some non-trivial repo)? I'm thinking I could try the |
|
This would also mean that criterion doesn't do its statistics right and/or isn't able to compensate for strange timings. For all I know, it deals with outliers and does the statistically correct thing. |
Maybe something is iffy with my environment. Could you run the benchmark on the latest commit and see how the performance looks for you? |
|
Here is the first run on Besides greater variance than I was remembering, the tests we are interested in 'improved' in the ballpark of the previous performance reduction. The second run seems to bring everything back to the original values. I only typed in the browser while it was running though. |
|
Maybe it would help to have a very specific benchmark for the EntryMode handling/parsing as well? |
OK. That gives me enough confidence to finish implementing the I may look into adding a new benchmark later, but let's first get to a plausible PR to evaluate. |
pierrechevalier83
force-pushed
the
issue_1887
branch
2 times, most recently
from
3b07d87 to
0f503db
Compare
|
After rebooting and running the benchmarks without running anything else, I get slightly more consistent results and they're not great: For
I think it means I should go ahead with the |
pierrechevalier83
force-pushed
the
issue_1887
branch
from
0f503db to
5e2b786
Compare
I tried that and it works. Currently, I've got a POC branch (issue_1887_compact) where I get mostly improved benchmarks except I'll continue tomorrow: I'll resolve this small pessimization and deal with the rest of the propagation to other crates, then make a PR that fits within the conventions. |
|
I went ahead and fixed the pessimization here. Benchmark results:
I'll turn it into a real PR tomorrow. |
`clippy` has recently begun to fail with:
error: unnecessary semicolon
--> gix-transport/src/client/blocking_io/http/traits.rs:30:18
|
30 | };
| ^ help: remove
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
= note: `-D clippy::unnecessary-semicolon` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_semicolon)]`
While it looks like this might first have been observed in GitoxideLabs#1917,
it is unrelated to any change there. It happens when the current
tip of main (4660f7a) is rerun, as observed in:
https://github.com/EliahKagan/gitoxide/actions/runs/14254079128/job/39958292846
`clippy` has recently begun to fail with:
error: unnecessary semicolon
--> gix-transport/src/client/blocking_io/http/traits.rs:30:18
|
30 | };
| ^ help: remove
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
= note: `-D clippy::unnecessary-semicolon` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_semicolon)]`
While it looks like this might first have been observed in GitoxideLabs#1917,
it is unrelated to any change there. It happens when the current
tip of main (4660f7a) is rerun, as observed in:
https://github.com/EliahKagan/gitoxide/actions/runs/14254079128/job/39958292846
This removes an extra unnecessary semicolon that `clippy` has
recently begun to catch, causing CI to fail with:
error: unnecessary semicolon
--> gix-transport/src/client/blocking_io/http/traits.rs:30:18
|
30 | };
| ^ help: remove
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
= note: `-D clippy::unnecessary-semicolon` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_semicolon)]`
While it looks like this might first have been observed in GitoxideLabs#1917,
it is unrelated to any change there. It happens when the current
tip of main (4660f7a) is rerun, as observed in:
https://github.com/EliahKagan/gitoxide/actions/runs/14254079128/job/39958292846
|
That's awesome, thanks, I am looking forward to trying it myself then. |
This removes some extra unnecessary semicolons that `clippy` has
recently begun to catch, causing CI to fail with errors such as:
error: unnecessary semicolon
--> gix-transport/src/client/blocking_io/http/traits.rs:30:18
|
30 | };
| ^ help: remove
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
= note: `-D clippy::unnecessary-semicolon` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_semicolon)]`
While it looks like this might first have been observed in GitoxideLabs#1917,
it is unrelated to any change there. It happens when the current
tip of main (4660f7a) is rerun, as observed in:
https://github.com/EliahKagan/gitoxide/actions/runs/14254079128/job/39958292846
This removes some extra unnecessary semicolons that `clippy` has
recently begun to catch, causing CI to fail with errors such as:
error: unnecessary semicolon
--> gix-transport/src/client/blocking_io/http/traits.rs:30:18
|
30 | };
| ^ help: remove
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_semicolon
= note: `-D clippy::unnecessary-semicolon` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_semicolon)]`
While it looks like this might first have been observed in GitoxideLabs#1917,
it is unrelated to any change there. It happens when the current
tip of main (4660f7a) is rerun, as observed in:
https://github.com/EliahKagan/gitoxide/actions/runs/14254079128/job/39958292846
This runs `just clippy-fix` and `etc/copy-packetline.sh` to fix it.
pierrechevalier83
force-pushed
the
issue_1887
branch
from
5e2b786 to
88adb57
Compare
It's despite the fix. I saved some cycles in refactorings/small optimizations, which allows to absorb the necessary added cost. I've updated this PR now with the compact version of the code. I also layed out the diffs so that the behaviour change stands out in a short self-contained commit. |
pierrechevalier83
force-pushed
the
issue_1887
branch
3 times, most recently
from
be1d4c7 to
ea655f3
Compare
pierrechevalier83
changed the title
Change the gix-object API so that client code can't explicitely rely on
the internal representation of `EntryMode`.
This is necessary as we need to change it to allow round-trip behaviour
for modes like `b"040000"` and `b"40000"` which currently share the
`0o40000u16` representation.
Use the opportunity to sprinkle a couple of optimizations in the parsing
of the EntryMode since we had to go deep in understanding this code
anyway, so may as well. Mostly, don't `reverse` the bytes when parsing.
```
TreeRefIter() time: [46.251 ns 46.390 ns 46.549 ns]
change: [-17.685% -16.512% -15.664%] (p = 0.00 < 0.05)
Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
```
Also add a test that shows the current incorrect behaviour.
Change the internal representation of `EntryMode` to fix a round-trip
issue.
Prior to this change, both `b"040000"` and `b"40000"` mapped to
`0o40000u16`.
After this change,
* `b"040000"` is represented by `0o140000`
* `b"40000"` is represented by `0o40000`
*Tests*:
We can see in the `as_bytes` test that the behaviour is fixed now.
We also add a test to show we now can round-trip on the existing test
fixtures which contain examples of this situation.
*Performance*:
We pay a cost for this compared to the parent commit:
```
TreeRefIter() time: [50.403 ns 50.611 ns 50.830 ns]
change: [+8.6240% +9.0871% +9.5776%] (p = 0.00 < 0.05)
Performance has regressed.
```
but we already did enough optimizations to pay for this earlier in this
PR:
* `main`: `~55 ns`
* `parent`: `~46 ns`
* `this commit`: `~50 ns`
Fixes 1887
There was a problem hiding this comment.
Thanks a million, this is great work! This PR improves on the usage of EntryMode as well which is very visible in the adapt … commit(s).
I took the liberty to refactor the commits and the subject-style to help driving cargo smart-release. With fix(crate)!: it won't pick it up as conventional, hence it's ineffective to indicate a breaking change.
I also saw that related PRs were coming up, I will get to them shortly.
|
Does this still treat all cases of repositories with strange type and mode values (that is, where the actual numerical value is not one of the values that is expected to be found represented in a tree object) the same as before? |
|
Even though I am sure @pierrechevalier83 will have a more qualified response, I'd say yes it does here. From what I can tell, it's not worse than before, while encoding a known special case into the u16. My concern is that technically, there can be many more strangely formed modes that would still not round-trip even though Git can handle them (better). |
Outside for a hypothetical
I share that concern. I think two situations could possibly happen that would be problematic:
Any other strange situations would imply the byte string for the mode is made of not only digits or more than 6 digits. I'm hopeful that these really don't exist in the wild, but if they do, we'll need to byte the bullet and change the backing to
FYI, we currently have slightly over 7000 Git repos at Meta hosted on Mononoke, backed by
Thanks a million to you. I really appreciate the time you took guiding me through the solution space with all the deep contextual knowledge you have. It's also really pleasant to benefit from a well tested and benchmarked codebase to begin with so that mistakes are easier to identify and correct. Mononoke gets a lot of value from gitoxide, so contributing back a little bit serves our interest and is the least we can do. Win-win :) |
Before this change, the
EntryModethat represents aTreecould be represented asb"40000"orb"040000", and the difference would get lost once it wasrepresented as
0o40000u16.We fix it by representing
"b040000"as0o140000, which is safe to do because0o140000cannot represent a validEntryModein the internal representation ofEntryMode.This internal representation is not exposed to any client code.
Fixes issue #1887