Re: [PATCH v5 4/4] submodule: record superproject gitdir during 'update'

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 08 2021, Emily Shaffer wrote:

> On Fri, Nov 05, 2021 at 09:43:56AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> 
>> On Thu, Nov 04 2021, Junio C Hamano wrote:
>> 
>> > Emily Shaffer <emilyshaffer@google.com> writes:
>> >
>> >> A recorded hint path to the superproject's gitdir might be added during
>> >> 'git submodule add', but in some cases - like submodules which were
>> >> created before 'git submodule add' learned to record that info - it might
>> >> be useful to update the hint. Let's do it during 'git submodule
>> >> update', when we already have a handle to the superproject while calling
>> >> operations on the submodules.
>> >
>> > We are hearing repeated mention of "cache" and "hint".  Do we ever
>> > invalidate it, or if we have such a record, do we blindly trust it
>> > and use it without verifying if it is still fresh?
>> >
>> > Also, this step and the previous step both say we record gitdir on
>> > their title, but we instead record common dir.  Whichever is the
>> > right choice to record, let's be consistent.
>> 
>> I had similar (AFAICT still unaddressed) feedback on the v2[1]. I'd lost
>> track of this series, and see one reason is that the In-Reply-Chain was
>> broken between v3..v4.
>> 
>> I.e. it seems to me that this whole thing started as a way to avoid
>> shellscript overhead by calling git-rev-parse from git-submodule.sh, but
>> now that the relevant bits are moved to C we could just call some
>> slightly adjusted code in setup.c.
>
> No, that is not the case. It is the case that `git -C .. rev-parse
> --git-dir` is *very* expensive in the case where `../` is not, in fact,
> a gitdir; when I attempted another series which relied on finding the
> parent superproject's gitdir in this way, our testsuite took something
> like 5x longer to run than before. In other words, the expensive part is
> not the shelling out overhead - the expensive part is searching up the
> entire filesystem directory structure in the worst-case ("we are not a
> submodule") scenario. This is still needed, even with 'git-submodule.sh'
> moving to C.

Do you have that test code somewhere? I tried to reproduce this &
can't. I run my tests in /home/avar/*, and just created this:

    $ find /tmp/some/ -name '.git' -type d
    /tmp/some/dir/.git
    /tmp/some/dir/a/b/c/d/e/f/g/i/j/k/.git

I.e. a deeply nested structure in /tmp, if you ask for the git-dir in
/tmp/some/**/k you'll need to search several levels up.

Then with the patch below we'll instrument almost all git commands to
optionally do that search, i.e. anything that does setup_git_directory()
at all:

    $ GIT_TEST_SETUP=true GIT_TEST_SETUP_PRINT=true ~/g/git/git rev-parse HEAD
    warning: from '/tmp/some/dir/a/b/c/d/e/f/g/i/j' found '/tmp/some/dir' ('/tmp/some/dir/.git')
    <some hash here>

And as a quick test to run a few tests I tried:

    rm -rf test-results/; GIT_TEST_SETUP=true GIT_TEST_SETUP_PRINT=true prove -j8 t741*submod*.sh :: -V

Which runs quickly enough for a tight test loop, and does that work >600 times:
    
    $ cat test-results/*.out|grep -c warning.*from.*found
    662

I can't get that to show me any meaningful difference, just to pick on
one test (since it was easier to run repeatedly):
    
    $ hyperfine -L v true,false "GIT_TEST_SETUP={v} ./t7416-submodule-dash-url.sh --root=/dev/shm/git"
    Benchmark #1: GIT_TEST_SETUP=true ./t7416-submodule-dash-url.sh --root=/dev/shm/git
      Time (mean ± σ):     527.5 ms ±   7.2 ms    [User: 431.6 ms, System: 125.9 ms]
      Range (min … max):   522.6 ms … 542.5 ms    10 runs
     
    Benchmark #2: GIT_TEST_SETUP=false ./t7416-submodule-dash-url.sh --root=/dev/shm/git
      Time (mean ± σ):     526.7 ms ±  10.8 ms    [User: 421.1 ms, System: 131.6 ms]
      Range (min … max):   518.2 ms … 546.8 ms    10 runs
     
    Summary
      'GIT_TEST_SETUP=false ./t7416-submodule-dash-url.sh --root=/dev/shm/git' ran
        1.00 ± 0.02 times faster than 'GIT_TEST_SETUP=true ./t7416-submodule-dash-url.sh --root=/dev/shm/git'

I.e. it's all fuzzy and within the error margins.

Now if we do e.g.:

    GIT_TEST_SETUP=false strace -f -c -U calls,name,time -S calls ./t7416-submodule-dash-url.sh 2>&1 >/dev/null | grep -A9000 calls >a
    GIT_TEST_SETUP=true strace -f -c -U calls,name,time -S calls ./t7416-submodule-dash-url.sh 2>&1 >/dev/null | grep -A9000 calls >b

We'll see the syscall difference, in summary:

    -   110086 total            100.00
    +   114765 total            100.00

And some of the real big differences are:
    
    $ diff -u <(head -n 12 a) <(head -n 12 b)
    --- /dev/fd/63  2021-11-09 01:57:16.023991556 +0100
    +++ /dev/fd/62  2021-11-09 01:57:16.019991593 +0100
    @@ -1,12 +1,12 @@
         calls syscall          % time
     --------- ---------------- ------
    -    11504 openat             2.15
    -    11496 close              2.07
    -    10672 read               4.15
    -    10465 rt_sigaction       0.32
    -     9913 lstat              1.50
    -     9456 mmap               0.67
    -     7545 stat               0.81
    -     6349 fstat              0.62
    -     3896 access             0.61
    -     3490 mprotect           0.26
    +    11783 lstat              1.80
    +    11600 close              1.89
    +    11599 openat             2.19
    +    10887 read               4.36
    +    10465 rt_sigaction       0.33
    +     9742 stat               1.07
    +     9455 mmap               0.75
    +     6346 fstat              0.57
    +     4113 access             0.65
    +     3490 mprotect           0.28

But syscalls are fast, so it doesn't show up in real results.

Now, of course a real implementation could be less stupid, e.g. even if
we think we need *a cache* if these are the performance numbers why do
we need to risk the cache being incorrect v.s. say just writing "I am a
submodule" somewhere in the config (if we don't have that).

Then we'll only do that work for submodules, so not all git invocations
will pay the cost (and it this point we'll usually have read config
anyway).

But I really just don't think it's that expensive at all. I can see how
it would be for actually shelling out, but we don't need to do that.

This could also just be that I'm running this on a really fast FS, which
is true. So I went and tested on an AIX machine I have access to.

I/O on AIX is slow, *really slow*, so slow that if you "rm -rfv"
something you'll have time to read individual lines scrolling by.

That ~500ms t7416-submodule-dash-url.sh test runs in around 50s on that
AIX box (power-aix.osuosl.org), most of which is I/O overhead, I created
the same /tmp/ directory structure and tried with
GIT_TEST_SETUP=[false|true] and it's ~55s with/without the env variable,
with no clear winner.

I don't have access to hyperfine on that box, or the patience to wait
for AIX I/O to wait for meaningful results, but to a first approximation
that seems to indicate that it doesn't really matter there either.

diff --git a/setup.c b/setup.c
index 347d7181ae9..8453d397676 100644
--- a/setup.c
+++ b/setup.c
@@ -1209,6 +1209,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
        struct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;
        const char *prefix = NULL;
        struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+       const char *str = "/tmp/some/dir/a/b/c/d/e/f/g/i/j";
+       struct strbuf a = STRBUF_INIT, b = STRBUF_INIT;
 
        /*
         * We may have read an incomplete configuration before
@@ -1231,6 +1233,14 @@ const char *setup_git_directory_gently(int *nongit_ok)
                die_errno(_("Unable to read current working directory"));
        strbuf_addbuf(&dir, &cwd);
 
+       if (git_env_bool("GIT_TEST_SETUP", 0)) {
+               strbuf_addstr(&a, str);
+               setup_git_directory_gently_1(&a, &b, 0);
+
+               if (strcmp(a.buf, str) && git_env_bool("GIT_TEST_SETUP_PRINT", 0))
+                       warning("from '%s' found '%s' ('%s/%s')", str, a.buf, a.buf, b.buf);
+       }
+
        switch (setup_git_directory_gently_1(&dir, &gitdir, 1)) {
        case GIT_DIR_EXPLICIT:
                prefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);