"submodule foreach" much slower than removed "submodule--helper --list"

"submodule foreach" much slower than removed "submodule--helper --list"

[Jonas Bernoulli]

In v2.38.0 (31955475d1c283120d5d84247eb3fd55d9f5fdd9)
"submodule--helper --list" was remove because

> We're not getting anything useful from the "list | cut -f2"
> invocation that we couldn't get from "foreach 'echo $sm_path'".

But we get speed (this is with about one hundred modules):

$ time git submodule foreach -q 'echo $sm_path' > /dev/null

real    0m0.585s
user    0m0.413s
sys     0m0.182s

$ time git submodule--helper list > /dev/null

real    0m0.008s
user    0m0.004s
sys     0m0.004s

Please consider restoring this subcommand or providing something
equivalent that is just as fast.

     Thanks,
     Jonas

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jonas Bernoulli]

I just noticed that "submodule--helper name" was also removed, which I
also found useful in scripts.  Please tell me if I am missing something,
but it seems I now have to do something like this instead:

  git config -f .gitmodules --list |
      sed -n "s|^submodule.\([^.]*\).path=$path\$|\1|p"

The old way was nicer:

  git submodule--helper name $path

I realize submodule--helper is for internal use and using it anyway
comes with the risk of such removals and other changes, but again,
please consider restoring that or providing something similar in the
public interface.

     Cheers,
     Jonas

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jeff King]

On Sat, Oct 15, 2022 at 06:50:36PM +0200, Jonas Bernoulli wrote:

> > We're not getting anything useful from the "list | cut -f2"
> > invocation that we couldn't get from "foreach 'echo $sm_path'".
> 
> But we get speed (this is with about one hundred modules):
> 
> $ time git submodule foreach -q 'echo $sm_path' > /dev/null
> 
> real    0m0.585s
> user    0m0.413s
> sys     0m0.182s
> 
> $ time git submodule--helper list > /dev/null
> 
> real    0m0.008s
> user    0m0.004s
> sys     0m0.004s

Yeah, that's not too surprising that it's slower. It's exec-ing a bunch
of shells to do that.

I don't really use submodules and haven't worked much on them either,
but maybe one of these works:

  - the output of the old "submodule--helper list" looks a lot like
    "ls-files" dumping the index and filtering on submodule entries.
    Running:

       git ls-files -s | grep ^160000

    produces the same output.

  - If you just want the oid/path of each, then "git submodule status"
    prints those. You might want "--cached" to avoid spending extra time
    checking the status (though it kind of looks like we may run "git
    describe" in each anyway; yikes!)

I'm not sure if those are exactly equivalent, either. It looks like the
old code was probably respecting submodule active markers (though in my
test repo without the submodule actually checked out, it's still
reported). There is probably room for a user-facing "git submodule list"
command, but again, I don't really know enough about the space to say
what it should report.

-Peff

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, that's not too surprising that it's slower. It's exec-ing a bunch
> of shells to do that.
> ...
> I'm not sure if those are exactly equivalent, either. It looks like the
> old code was probably respecting submodule active markers (though in my
> test repo without the submodule actually checked out, it's still
> reported). There is probably room for a user-facing "git submodule list"
> command, but again, I don't really know enough about the space to say
> what it should report.

We could mimic "find" and make "foreach" that has no action default
to an equivalent of "echo" that is done internally without forking.

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jeff King]

On Sat, Oct 15, 2022 at 11:16:06AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, that's not too surprising that it's slower. It's exec-ing a bunch
> > of shells to do that.
> > ...
> > I'm not sure if those are exactly equivalent, either. It looks like the
> > old code was probably respecting submodule active markers (though in my
> > test repo without the submodule actually checked out, it's still
> > reported). There is probably room for a user-facing "git submodule list"
> > command, but again, I don't really know enough about the space to say
> > what it should report.
> 
> We could mimic "find" and make "foreach" that has no action default
> to an equivalent of "echo" that is done internally without forking.

That would be reasonable to me, though I wonder what the output format
should be. Just name, or name/oid? Once you start having options, you
might as well just add a separate "git submodule list" that can take
them.

-Peff

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> We could mimic "find" and make "foreach" that has no action default
>> to an equivalent of "echo" that is done internally without forking.
>
> That would be reasonable to me, though I wonder what the output format
> should be. Just name, or name/oid? Once you start having options, you
> might as well just add a separate "git submodule list" that can take
> them.

Users of "find" seem to be happy with "-print" being the default, so
whatever we were doing with the internal "helper --list" would be
appropriate here.  Folks who want different output can feed their
own action.

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   - the output of the old "submodule--helper list" looks a lot like
>     "ls-files" dumping the index and filtering on submodule entries.
>     Running:
>
>        git ls-files -s | grep ^160000
>
>     produces the same output.

Indeed that was what we wrote in the scripted Porcelain.  This is
one of the times I wish we didn't turn things to C in piecemeal.

> I'm not sure if those are exactly equivalent, either. It looks like the
> old code was probably respecting submodule active markers (though in my
> test repo without the submodule actually checked out, it's still
> reported).

"submodule--helper list" being the "give me the gitlinks", to be
filtered with things like pathspec and presence of .git (i.e. being
activated), you are right that those who liked the output from it
would not be happy with "foreach" anyway.  The comparison in the
original was apples and oranges because "foreach" should show only
the ones that are active in the current checkout.

If one likes the output from "submodule--helper list" so much, I
think your "ls-files" above should be the closest.  There seems to
have existed some logic to squash unmerged entries down to a single
one, too (git-submodule.sh in Git 2.0.0 era has a module_list shell
function that shows what "helper list" should be doing), though.

Thanks.

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jeff King]

On Sat, Oct 15, 2022 at 03:40:16PM -0700, Junio C Hamano wrote:

> If one likes the output from "submodule--helper list" so much, I
> think your "ls-files" above should be the closest.  There seems to
> have existed some logic to squash unmerged entries down to a single
> one, too (git-submodule.sh in Git 2.0.0 era has a module_list shell
> function that shows what "helper list" should be doing), though.

Ah, good digging. I briefly looked at the patch removing the
submodule--helper version, saw the word "active", and assumed it was
checking that. On closer inspection, it is just the global "active_nr"
for iterating over the index. ;)

So yeah, using "ls-files" is a suitable replacement for Jonas's original
complaint. I'll leave it up to folks more interested and knowledgeable
on submodules to discuss whether there ought to be a listing command
that's more aware of gitmodules, populated fields, active flags, etc.

-Peff

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Matti Möll]

On 16.10.22 00:40, Junio C Hamano wrote:
> 
> If one likes the output from "submodule--helper list" so much, I
> think your "ls-files" above should be the closest.  There seems to
> have existed some logic to squash unmerged entries down to a single
> one, too (git-submodule.sh in Git 2.0.0 era has a module_list shell
> function that shows what "helper list" should be doing), though.

I do actually like that submodule is hoisted out of the scripts but as a 
matter of fact it seems that the openembedded folks relied on the 
"submodule--helper list" call and with later git versions that doesn't 
work anymore.

https://github.com/openembedded/openembedded-core/commit/6d9364e5f3535954f65cbbc694ee7933ac1d664f

At the end of the day it's kinda their fault to depend on the 
submodule--helper but it does hinder adoption of new git versions for 
people using openembedded.

Cheers,

		Matti

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Taylor Blau]

On Mon, Nov 07, 2022 at 12:01:40PM +0100, Matti Möll wrote:
> On 16.10.22 00:40, Junio C Hamano wrote:
> >
> > If one likes the output from "submodule--helper list" so much, I
> > think your "ls-files" above should be the closest.  There seems to
> > have existed some logic to squash unmerged entries down to a single
> > one, too (git-submodule.sh in Git 2.0.0 era has a module_list shell
> > function that shows what "helper list" should be doing), though.
>
> I do actually like that submodule is hoisted out of the scripts but as a
> matter of fact it seems that the openembedded folks relied on the
> "submodule--helper list" call and with later git versions that doesn't work
> anymore.
>
> https://github.com/openembedded/openembedded-core/commit/6d9364e5f3535954f65cbbc694ee7933ac1d664f
>
> At the end of the day it's kinda their fault to depend on the
> submodule--helper but it does hinder adoption of new git versions for people
> using openembedded.

Yeah, I agree. It's unfortunate, but I think we consider anything with
'--' in the builtin name to be outside of our compatibility guarantee.

I recall a recent message on the list which where Junio said as much,
but I can't seem to find it :-<.

Thanks,
Taylor

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 07 2022, Taylor Blau wrote:

> On Mon, Nov 07, 2022 at 12:01:40PM +0100, Matti Möll wrote:
>> On 16.10.22 00:40, Junio C Hamano wrote:
>> >
>> > If one likes the output from "submodule--helper list" so much, I
>> > think your "ls-files" above should be the closest.  There seems to
>> > have existed some logic to squash unmerged entries down to a single
>> > one, too (git-submodule.sh in Git 2.0.0 era has a module_list shell
>> > function that shows what "helper list" should be doing), though.
>>
>> I do actually like that submodule is hoisted out of the scripts but as a
>> matter of fact it seems that the openembedded folks relied on the
>> "submodule--helper list" call and with later git versions that doesn't work
>> anymore.
>>
>> https://github.com/openembedded/openembedded-core/commit/6d9364e5f3535954f65cbbc694ee7933ac1d664f
>>
>> At the end of the day it's kinda their fault to depend on the
>> submodule--helper but it does hinder adoption of new git versions for people
>> using openembedded.
>
> Yeah, I agree. It's unfortunate, but I think we consider anything with
> '--' in the builtin name to be outside of our compatibility guarantee.
>
> I recall a recent message on the list which where Junio said as much,
> but I can't seem to find it :-<.

We've never even documented it, so the uses that have been made of it
have been the result of source spelunking. 

B.t.w. the openembedded commit above using "git config" is not going to
get the same results as "git submodule--helper list", or the equivalent
"git submodule foreach", but perhaps it works well enough for what they
need it for...

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Ævar Arnfjörð Bjarmason]

Replying to both this & the parent post:

On Sat, Oct 15 2022, Jonas Bernoulli wrote:

> In v2.38.0 (31955475d1c283120d5d84247eb3fd55d9f5fdd9)
> "submodule--helper --list" was remove because
>
>> We're not getting anything useful from the "list | cut -f2"
>> invocation that we couldn't get from "foreach 'echo $sm_path'".
>
> But we get speed (this is with about one hundred modules):
>
> $ time git submodule foreach -q 'echo $sm_path' > /dev/null
>
> real    0m0.585s
> user    0m0.413s
> sys     0m0.182s
>
> $ time git submodule--helper list > /dev/null
>
> real    0m0.008s
> user    0m0.004s
> sys     0m0.004s
>
> Please consider restoring this subcommand or providing something
> equivalent that is just as fast.

Sorry about the slowdown, the removal of "list" was just an in-between
step to migrating "submodule" to a full built-in.

I can't reproduce anything like the 8ms v.s. ~600ms difference you note
here, but for me migrating it to a built-in is around 10% slower with
"foreach" than the old "list". I wonder what results you get?

I sent in a topic to migrate it since you sent this report. I was going
to do it in this development cycle, but this prompted me to do it
earlier:

	https://lore.kernel.org/git/cover-00.10-00000000000-20221017T115544Z-avarab@gmail.com/

On Sat, Oct 15 2022, Jonas Bernoulli wrote:

> I just noticed that "submodule--helper name" was also removed, which I
> also found useful in scripts.  Please tell me if I am missing something,
> but it seems I now have to do something like this instead:
>
>   git config -f .gitmodules --list |
>       sed -n "s|^submodule.\([^.]*\).path=$path\$|\1|p"
>
> The old way was nicer:
>
>   git submodule--helper name $path
>
> I realize submodule--helper is for internal use and using it anyway
> comes with the risk of such removals and other changes, but again,
> please consider restoring that or providing something similar in the
> public interface.

This however is another case, I removed "name" along with "list" and
other leftover code we weren't using anymore for the internal-only
"submodule--helper" (which is at turns out, was not as internal-only as
we'd hoped).

For "list" it's clear how to use "foreach" instead, but for "name" then
AFAICT the "best" replacement is to do a:

	git submodule foreach 'echo $displaypath $name'

And pipe that into grep/sed. If that's fast enough would it satisfy your
use-case, or would a "name" equivalent be handy?

I think the best way to prove that would be e.g.:

	git submodule foreach-format '%{name}' -- <pathspec>

Which, due to the "foreach" taking N number of arguments isn't easy to
add to "foreach" without the interface becoming somewhat tortured (we
could add a [---pathspec=<pathspec>]...), but "-- <pathspec>" with a
different subcommand name seems better.

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jeff King]

On Mon, Oct 17, 2022 at 06:50:06PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Sorry about the slowdown, the removal of "list" was just an in-between
> step to migrating "submodule" to a full built-in.
> 
> I can't reproduce anything like the 8ms v.s. ~600ms difference you note
> here, but for me migrating it to a built-in is around 10% slower with
> "foreach" than the old "list". I wonder what results you get?

I don't think "foreach" can ever be as performant as a bare "list". It
inherently is going to do O(n) fork+execs. So it's OK if you have one or
two submodules, but not if you have hundreds.

If it treated "echo" as a builtin and served it in-process, it could
avoid that extra overhead. But there are so many corner cases there,
you'd probably do better to add "list --format" instead.

-Peff

Re: "submodule foreach" much slower than removed "submodule--helper --list"

[Jonas Bernoulli]

Sorry for going silent right after bringing this up.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I can't reproduce anything like the 8ms v.s. ~600ms difference you note
> here, but for me migrating it to a built-in is around 10% slower with
> "foreach" than the old "list". I wonder what results you get?

The repository, in which I observed this slowdown, has one hundred
modules.

> I sent in a topic to migrate it since you sent this report. I was going
> to do it in this development cycle, but this prompted me to do it
> earlier:

Thanks!

A lot more is happening here than I can quickly understand, but the last
commit mentions that the slowdown is now just 0.1, which would be good
enough for me, I think.

> On Sat, Oct 15 2022, Jonas Bernoulli wrote:
>
>> I just noticed that "submodule--helper name" was also removed, which I
>> also found useful in scripts.  Please tell me if I am missing something,
>> but it seems I now have to do something like this instead:
>>
>>   git config -f .gitmodules --list |
>>       sed -n "s|^submodule.\([^.]*\).path=$path\$|\1|p"
>>
>> The old way was nicer:
>>
>>   git submodule--helper name $path
>>
>> I realize submodule--helper is for internal use and using it anyway
>> comes with the risk of such removals and other changes, but again,
>> please consider restoring that or providing something similar in the
>> public interface.
>
> This however is another case, I removed "name" along with "list" and
> other leftover code we weren't using anymore for the internal-only
> "submodule--helper" (which is at turns out, was not as internal-only as
> we'd hoped).
>
> For "list" it's clear how to use "foreach" instead, but for "name" then
> AFAICT the "best" replacement is to do a:
>
> 	git submodule foreach 'echo $displaypath $name'
>
> And pipe that into grep/sed. If that's fast enough would it satisfy your
> use-case, or would a "name" equivalent be handy?
>
> I think the best way to prove that would be e.g.:
>
> 	git submodule foreach-format '%{name}' -- <pathspec>
>
> Which, due to the "foreach" taking N number of arguments isn't easy to
> add to "foreach" without the interface becoming somewhat tortured (we
> could add a [---pathspec=<pathspec>]...), but "-- <pathspec>" with a
> different subcommand name seems better.

I agree, that adding support for "-- <pathspec>" to an existing or new
subcommand, would make it unnecessary to bring back a "name" subcommand.

Will "foreach"/"foreach-format" continue to be limited to active
modules?  Sometimes it would be nice to list all modules, including
those that are inactive.  As mentioned earlier "git ls-files -s | grep
^160000" is enough to get a list of the module paths, but sometimes we
want more information, e.g., "git submodule list --include-inactive
--format '$name $is_active submodule.$name.url' -- <pathspec>".

     Jonas