Re: [PATCH v3 3/5] sha1_name: Unroll len loop in find_unique_abbrev_r
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Oct 4, 2017, 13:06 UTC
- Message-ID
- <ff54de0c-cd7c-bc3d-dd18-5fc248f3f573@gmail.com>
- In-Reply-To
- <xmqqpoa3cujz.fsf@gitster.mtv.corp.google.com>
On 10/4/2017 2:10 AM, Junio C Hamano wrote:
Show 14 quoted lines
> Derrick Stolee <stolee@gmail.com> writes: > ... >> I understand that this patch on its own does not have good numbers. I >> split the >> patches 3 and 4 specifically to highlight two distinct changes: >> >> Patch 3: Unroll the len loop that may inspect all files multiple times. >> Patch 4: Parse less while disambiguating. >> >> Patch 4 more than makes up for the performance hits in this patch. > Now you confused me even more. When we read the similar table that > appears in [Patch 4/5], what does the "Base Time" column mean? > Vanilla Git with [Patch 3/5] applied? Vanillay Git with [Patch 4/5] > alone applied? Something else?
In PATCH 3, 4, and 5, I used the commit-by-commit diff for the perf numbers, so the "Base Time" for PATCH 4 is the time calculated when PATCH 3 is applied. The table in the [PATCH 0/5] message includes the relative change for all commits.
I recalculated the relative change for each patch related to the baseline (PATCH 2). Looking again, it appears I misspoke and PATCH 4 does include a +8% change for a fully-repacked Linux repo relative to PATCH 2. Since PATCH 5 includes an optimization targeted directly at large packfiles, the final performance gain is significant in the fully-packed cases.
It is also worth looking at the absolute times for these cases, since the fully-packed case is significantly faster than the multiple-packfile case, so the relative change impacts users less.
One final note: the improvement was clearer in test p0008.1 when the test included "sort -R" to shuffle the known OIDs. Providing OIDs in lexicographic order has had a significant effect on the performance, which does not reflect real-world usage. I removed the "sort -R" because it is a GNU-ism, but if there is a good cross-platform alternative I would be happy to replace it.
p0008.1: find_unique_abbrev() for existing objects --------------------------------------------------
For 10 repeated tests, each checking 100,000 known objects, we find the following results when running in a Linux VM:
| Repo | Baseline | Patch 3 | Rel % | Patch 4 | Rel % | Patch 5 | Rel % | |-------|----------|---------|-------|---------|-------|---------|-------| | Git | 0.09 | 0.06 | -33% | 0.05 | -44% | 0.05 | -44% | | Git | 0.11 | 0.08 | -27% | 0.08 | -27% | 0.08 | -27% | | Git | 0.09 | 0.07 | -22% | 0.06 | -33% | 0.06 | -33% | | Linux | 0.13 | 0.32 | 146% | 0.14 | + 8% | 0.05 | -62% | | Linux | 1.13 | 1.12 | - 1% | 0.94 | -17% | 0.88 | -22% | | Linux | 1.08 | 1.05 | - 3% | 0.86 | -20% | 0.80 | -26% | | VSTS | 0.12 | 0.23 | +92% | 0.11 | - 8% | 0.05 | -58% | | VSTS | 1.02 | 1.08 | + 6% | 0.95 | - 7% | 0.95 | - 7% | | VSTS | 2.25 | 2.08 | - 8% | 1.82 | -19% | 1.93 | -14% |
(Each repo has three versions, in order: 1 packfile, multiple packfiles, and multiple packfiles and loose objects.)
Thanks, -Stolee