threads / discuss / 58629

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

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

## tl;dr

14 messages between Oct 15, 2022 and Nov 8, 2022.

replies: 13people: 6as markdown or json

Jonas Bernoulli· Oct 15, 2022, 16:50 UTC · lore

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
Jonas Bernoulli· Oct 15, 2022, 17:34 UTC · re: Jonas Bernoulli · lore

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

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
Jeff King· Oct 15, 2022, 18:13 UTC · re: Jonas Bernoulli · lore

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

On Sat, Oct 15, 2022 at 06:50:36PM +0200, Jonas Bernoulli wrote:
Show 16 quoted lines
> > 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
Junio C Hamano· Oct 15, 2022, 18:16 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.

Jeff King· Oct 15, 2022, 18:23 UTC · re: Junio C Hamano · lore

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

On Sat, Oct 15, 2022 at 11:16:06AM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> 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
Junio C Hamano· Oct 15, 2022, 18:37 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
>> 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.

Junio C Hamano· Oct 15, 2022, 22:40 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 7 quoted lines
>   - 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.
Jeff King· Oct 17, 2022, 17:02 UTC · re: Junio C Hamano · lore

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

On Sat, Oct 15, 2022 at 03:40:16PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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
Matti Möll· Nov 7, 2022, 11:01 UTC · re: Junio C Hamano · lore

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

On 16.10.22 00:40, Junio C Hamano wrote:
Show 6 quoted lines
> 
> 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
Taylor Blau· Nov 8, 2022, 02:50 UTC · re: Matti Möll · lore

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

On Mon, Nov 07, 2022 at 12:01:40PM +0100, Matti Möll wrote:
Show 18 quoted lines
> 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

Ævar Arnfjörð Bjarmason· Nov 8, 2022, 03:37 UTC · re: Taylor Blau · lore

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

On Mon, Nov 07 2022, Taylor Blau wrote:
Show 25 quoted lines
> 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...

Ævar Arnfjörð Bjarmason· Oct 17, 2022, 16:50 UTC · re: Jonas Bernoulli · lore

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

Replying to both this & the parent post:
On Sat, Oct 15 2022, Jonas Bernoulli wrote:
Show 22 quoted lines
> 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:
Show 15 quoted lines
> 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.

Jeff King· Oct 17, 2022, 17:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Mon, Oct 17, 2022 at 06:50:06PM +0200, Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> 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
Jonas Bernoulli· Oct 21, 2022, 14:58 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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.

Show 39 quoted lines
> 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

← back to recent threads