Re: [PATCH v2 0/3] Fix use of 'cache_bottom' in sparse index

From: Derrick Stolee <derrickstolee@github.com>
Date: Mar 21, 2022, 19:12 UTC
Message-ID: <b4803b58-018b-fb02-717f-95e48d6f844a@github.com>
In-Reply-To: <pull.1179.v2.git.1647532536.gitgitgadget@gmail.com>

On 3/17/2022 11:55 AM, Victoria Dye via GitGitGadget wrote:

Show 6 quoted lines

> To fix this, the 'cache_bottom' advancement is reinstated in
> 'mark_ce_used(...)', and instead it is disabled in 'unpack_callback(...)' if
> the tree in question is a sparse directory. This corrects both the
> non-"cache diff" cases and the 'unpack_index_entry(...)' cases while
> preventing the double-advancement 17a1bb570b originally intended to avoid
> (patch [2/3]).

Thank you for digging deep and finding the root cause here _and_ the
correct fix.

Show 29 quoted lines

> Finally, now that the cache bottom is advanced properly, we can revert the
> "performance improvement" introduced in f2a454e0a5 (unpack-trees: improve
> performance of next_cache_entry, 2021-11-29) that mitigated performance
> issues arising in 'next_cache_entry(...)' from the non-advancing
> 'cache_bottom' (patch [3/3]). The performance results in
> 'p2000-sparse-operations.sh' showed expected variability around 0% change in
> execution time (+/= 0.04s, depending on the command), with example results
> for potentially-affected commands below.
> 
> 'git reset'                      master            this_series                  
> ------------------------------------------------------------------------
> full-v3                          0.51(0.21+0.27)   0.50(0.21+0.25) -2.0%
> full-v4                          0.51(0.22+0.27)   0.50(0.21+0.24) -2.0%
> sparse-v3                        0.30(0.04+0.55)   0.28(0.04+0.50) -6.7%
> sparse-v4                        0.31(0.04+0.51)   0.29(0.04+0.51) -6.5%
> 
> 'git reset -- does-not-exist'    master            this_series                  
> ------------------------------------------------------------------------
> full-v3                          0.54(0.23+0.27)   0.55(0.22+0.28) +1.9%
> full-v4                          0.56(0.25+0.26)   0.54(0.24+0.26) -3.6%
> sparse-v3                        0.31(0.04+0.54)   0.31(0.04+0.50) +0.0%
> sparse-v4                        0.31(0.04+0.52)   0.31(0.04+0.50) +0.0%
> 
> 'git diff --cached'              master            this_series    
> -------------------------------------------------------------------------
> full-v3                          0.09(0.04+0.04)   0.09(0.04+0.04) +0.0%
> full-v4                          0.09(0.04+0.04)   0.09(0.04+0.04) +0.0%
> sparse-v3                        0.05(0.01+0.02)   0.05(0.01+0.03) +0.0%
> sparse-v4                        0.04(0.01+0.02)   0.04(0.01+0.02) +0.0%

I'm glad this also makes things simpler here. It's interesting that
it previously manifested only as a performance issue and not a
correctness issue.
  
I carefully read these patches and think they are ready to go.

Thanks, -Stolee