diff mbox series

Speed up fixincludes.

Message ID b37ffed3-893f-7421-a3b2-ff311250f151@suse.cz
State New
Headers show
Series Speed up fixincludes. | expand

Commit Message

Martin Liška Feb. 3, 2022, 2:56 p.m. UTC 
In my case:
$ rm ./stmp-fixinc ; time make -j16

takes 17 seconds, where I can reduce it easily with the suggested
change. Then I get to 11.2 seconds.

The scripts searches ~2500 folders in my case with total 20K header
files.

Ready to be installed?
Thanks,
Martin

fixincludes/ChangeLog:

	* fixinc.in: Use mkdir -p rather that a loop.
---
  fixincludes/fixinc.in | 10 +++-------
  1 file changed, 3 insertions(+), 7 deletions(-)

Comments

Jeff Law Feb. 3, 2022, 3:19 p.m. UTC | #1 
On 2/3/2022 7:56 AM, Martin Liška wrote:
> In my case:
> $ rm ./stmp-fixinc ; time make -j16
>
> takes 17 seconds, where I can reduce it easily with the suggested
> change. Then I get to 11.2 seconds.
>
> The scripts searches ~2500 folders in my case with total 20K header
> files.
>
> Ready to be installed?
> Thanks,
> Martin
>
> fixincludes/ChangeLog:
>
>     * fixinc.in: Use mkdir -p rather that a loop.
Don't we run the risk of overflowing ARG_MAX with this style invocation?
jeff
Martin Liška Feb. 3, 2022, 3:40 p.m. UTC | #2 
On 2/3/22 16:19, Jeff Law wrote:
> 
> 
> On 2/3/2022 7:56 AM, Martin Liška wrote:
>> In my case:
>> $ rm ./stmp-fixinc ; time make -j16
>>
>> takes 17 seconds, where I can reduce it easily with the suggested
>> change. Then I get to 11.2 seconds.
>>
>> The scripts searches ~2500 folders in my case with total 20K header
>> files.
>>
>> Ready to be installed?
>> Thanks,
>> Martin
>>
>> fixincludes/ChangeLog:
>>
>>     * fixinc.in: Use mkdir -p rather that a loop.
> Don't we run the risk of overflowing ARG_MAX with this style invocation?
> jeff
> 

You are correct, fixed in the attached V2.

What do you think now?

Thanks,
Martin
Andreas Schwab Feb. 3, 2022, 4:06 p.m. UTC | #3 
On Feb 03 2022, Martin Liška wrote:

> -mkdir $LIB/root

Why did you remote that line?
Martin Liška Feb. 3, 2022, 4:08 p.m. UTC | #4 
On 2/3/22 17:06, Andreas Schwab wrote:
> On Feb 03 2022, Martin Liška wrote:
> 
>> -mkdir $LIB/root
> 
> Why did you remote that line?
> 

That was an accidental removal, fixed in V2.

Martin
Andreas Schwab Feb. 3, 2022, 4:12 p.m. UTC | #5 
On Feb 03 2022, Martin Liška wrote:

> What do you think now?

xargs?
Jakub Jelinek Feb. 3, 2022, 4:12 p.m. UTC | #6 
On Thu, Feb 03, 2022 at 04:40:25PM +0100, Martin Liška wrote:
> --- a/fixincludes/fixinc.in
> +++ b/fixincludes/fixinc.in
> @@ -258,12 +258,23 @@ then echo "All directories (including links to directories):"
>       echo $all_dirs
>  fi
>  
> -for file in $all_dirs; do
> -  rm -rf $LIB/$file
> -  if [ ! -d $LIB/$file ]
> -  then mkdir $LIB/$file
> -  fi
> -done
> +ARG_MAX=`getconf ARG_MAX 2>/dev/null`
> +LENGTH=`expr length "$all_dirs" + 10`
> +
> +if test -n "$ARG_MAX" && test $LENGTH -lt $ARG_MAX
> +then
> +  cd $LIB
> +  mkdir -p $all_dirs
> +  cd ..
> +else
> +  for file in $all_dirs; do
> +    rm -rf $LIB/$file
> +    if [ ! -d $LIB/$file ]
> +    then mkdir $LIB/$file
> +    fi
> +  done
> +fi

Is getconf and expr length sufficiently portable?
Can't you use
echo "$all_dirs" | xargs mkdir -p
instead?

	Jakub
Martin Liška Feb. 3, 2022, 4:21 p.m. UTC | #7 
On 2/3/22 17:12, Jakub Jelinek wrote:
> On Thu, Feb 03, 2022 at 04:40:25PM +0100, Martin Liška wrote:
>> --- a/fixincludes/fixinc.in
>> +++ b/fixincludes/fixinc.in
>> @@ -258,12 +258,23 @@ then echo "All directories (including links to directories):"
>>        echo $all_dirs
>>   fi
>>   
>> -for file in $all_dirs; do
>> -  rm -rf $LIB/$file
>> -  if [ ! -d $LIB/$file ]
>> -  then mkdir $LIB/$file
>> -  fi
>> -done
>> +ARG_MAX=`getconf ARG_MAX 2>/dev/null`
>> +LENGTH=`expr length "$all_dirs" + 10`
>> +
>> +if test -n "$ARG_MAX" && test $LENGTH -lt $ARG_MAX
>> +then
>> +  cd $LIB
>> +  mkdir -p $all_dirs
>> +  cd ..
>> +else
>> +  for file in $all_dirs; do
>> +    rm -rf $LIB/$file
>> +    if [ ! -d $LIB/$file ]
>> +    then mkdir $LIB/$file
>> +    fi
>> +  done
>> +fi
> 
> Is getconf and expr length sufficiently portable?
> Can't you use
> echo "$all_dirs" | xargs mkdir -p
> instead?

Oh, even better!

Ready to be installed?
Thanks,
Martin

> 
> 	Jakub
>
Jeff Law Feb. 3, 2022, 5:19 p.m. UTC | #8 
On 2/3/2022 9:21 AM, Martin Liška wrote:
> On 2/3/22 17:12, Jakub Jelinek wrote:
>> On Thu, Feb 03, 2022 at 04:40:25PM +0100, Martin Liška wrote:
>>> --- a/fixincludes/fixinc.in
>>> +++ b/fixincludes/fixinc.in
>>> @@ -258,12 +258,23 @@ then echo "All directories (including links to 
>>> directories):"
>>>        echo $all_dirs
>>>   fi
>>>   -for file in $all_dirs; do
>>> -  rm -rf $LIB/$file
>>> -  if [ ! -d $LIB/$file ]
>>> -  then mkdir $LIB/$file
>>> -  fi
>>> -done
>>> +ARG_MAX=`getconf ARG_MAX 2>/dev/null`
>>> +LENGTH=`expr length "$all_dirs" + 10`
>>> +
>>> +if test -n "$ARG_MAX" && test $LENGTH -lt $ARG_MAX
>>> +then
>>> +  cd $LIB
>>> +  mkdir -p $all_dirs
>>> +  cd ..
>>> +else
>>> +  for file in $all_dirs; do
>>> +    rm -rf $LIB/$file
>>> +    if [ ! -d $LIB/$file ]
>>> +    then mkdir $LIB/$file
>>> +    fi
>>> +  done
>>> +fi
>>
>> Is getconf and expr length sufficiently portable?
>> Can't you use
>> echo "$all_dirs" | xargs mkdir -p
>> instead?
>
> Oh, even better!
Definitely.  I suspect getconf wouldn't have worked across all the hosts 
we support.


>
> Ready to be installed?
Yes, this version is much better :-)

jeff
Andreas Schwab Feb. 3, 2022, 6:44 p.m. UTC | #9 
On Feb 03 2022, Martin Liška wrote:

> +cd $LIB
> +echo "$all_dirs" | xargs mkdir -p
> +cd ..
> +

$LIB always contains slashes.
Jakub Jelinek Feb. 3, 2022, 6:49 p.m. UTC | #10 
On Thu, Feb 03, 2022 at 10:19:37AM -0700, Jeff Law wrote:
> > > Is getconf and expr length sufficiently portable?
> > > Can't you use
> > > echo "$all_dirs" | xargs mkdir -p
> > > instead?
> > 
> > Oh, even better!
> Definitely.  I suspect getconf wouldn't have worked across all the hosts we
> support.
> 
> 
> > 
> > Ready to be installed?
> Yes, this version is much better :-)

Looking at mkinstalldirs, there are comments like:
# Solaris 8's mkdir -p isn't thread-safe.  If you mkdir -p a/b and
# mkdir -p a/c at the same time, both will detect that a is missing,
# one will create a, then the other will try to create a and die with
# a "File exists" error.  This is a problem when calling mkinstalldirs
# from a parallel make.  We use --version in the probe to restrict
# ourselves to GNU mkdir, which is thread-safe.
case $dirmode in
  '')
    if mkdir -p --version . >/dev/null 2>&1 && test ! -d ./--version; then
      echo "mkdir -p -- $*"
      exec mkdir -p -- "$@"
    else
      # On NextStep and OpenStep, the 'mkdir' command does not
      # recognize any option.  It will interpret all options as
      # directories to create, and then abort because '.' already
      # exists.
      test -d ./-p && rmdir ./-p
      test -d ./--version && rmdir ./--version
    fi
    ;;

I guess we don't need thread safety as hopefully we don't run this from
multiple jobs concurrently, NextStep/OpenStep are probably long time dead,
but does mkdir -p work now everywhere?

	Jakub
Martin Liška Feb. 3, 2022, 9:13 p.m. UTC | #11 
On 2/3/22 19:44, Andreas Schwab wrote:
> On Feb 03 2022, Martin Liška wrote:
> 
>> +cd $LIB
>> +echo "$all_dirs" | xargs mkdir -p
>> +cd ..
>> +
> 
> $LIB always contains slashes.
> 

And what is the problem? You're too brief..

Martin
Marek Polacek Feb. 3, 2022, 9:29 p.m. UTC | #12 
On Thu, Feb 03, 2022 at 10:13:36PM +0100, Martin Liška wrote:
> On 2/3/22 19:44, Andreas Schwab wrote:
> > On Feb 03 2022, Martin Liška wrote:
> > 
> > > +cd $LIB
> > > +echo "$all_dirs" | xargs mkdir -p
> > > +cd ..
> > > +
> > 
> > $LIB always contains slashes.
> > 
> 
> And what is the problem? You're too brief..

I guess his point is that if you do
cd a/b/c/
then
cd ..
will not get you back to where you started.  Perhaps you could use
pushd/popd instead.

Marek
Jakub Jelinek Feb. 3, 2022, 9:51 p.m. UTC | #13 
On Thu, Feb 03, 2022 at 04:29:39PM -0500, Marek Polacek wrote:
> On Thu, Feb 03, 2022 at 10:13:36PM +0100, Martin Liška wrote:
> > On 2/3/22 19:44, Andreas Schwab wrote:
> > > On Feb 03 2022, Martin Liška wrote:
> > > 
> > > > +cd $LIB
> > > > +echo "$all_dirs" | xargs mkdir -p
> > > > +cd ..
> > > > +
> > > 
> > > $LIB always contains slashes.
> > > 
> > 
> > And what is the problem? You're too brief..
> 
> I guess his point is that if you do
> cd a/b/c/
> then
> cd ..
> will not get you back to where you started.  Perhaps you could use
> pushd/popd instead.

Or a subshell.

	Jakub
Martin Liška Feb. 4, 2022, 9:26 a.m. UTC | #14 
On 2/3/22 22:51, Jakub Jelinek wrote:
> On Thu, Feb 03, 2022 at 04:29:39PM -0500, Marek Polacek wrote:
>> On Thu, Feb 03, 2022 at 10:13:36PM +0100, Martin Liška wrote:
>>> On 2/3/22 19:44, Andreas Schwab wrote:
>>>> On Feb 03 2022, Martin Liška wrote:
>>>>
>>>>> +cd $LIB
>>>>> +echo "$all_dirs" | xargs mkdir -p
>>>>> +cd ..
>>>>> +
>>>>
>>>> $LIB always contains slashes.
>>>>
>>>
>>> And what is the problem? You're too brief..
>>
>> I guess his point is that if you do
>> cd a/b/c/
>> then
>> cd ..
>> will not get you back to where you started.  Perhaps you could use
>> pushd/popd instead.
> 
> Or a subshell.
> 
> 	Jakub
> 

I'm suggesting the following patch.

Ready to be installed?
Thanks,
Martin
Marek Polacek Feb. 4, 2022, 2:17 p.m. UTC | #15 
On Fri, Feb 04, 2022 at 10:26:07AM +0100, Martin Liška wrote:
> On 2/3/22 22:51, Jakub Jelinek wrote:
> > On Thu, Feb 03, 2022 at 04:29:39PM -0500, Marek Polacek wrote:
> > > On Thu, Feb 03, 2022 at 10:13:36PM +0100, Martin Liška wrote:
> > > > On 2/3/22 19:44, Andreas Schwab wrote:
> > > > > On Feb 03 2022, Martin Liška wrote:
> > > > > 
> > > > > > +cd $LIB
> > > > > > +echo "$all_dirs" | xargs mkdir -p
> > > > > > +cd ..
> > > > > > +
> > > > > 
> > > > > $LIB always contains slashes.
> > > > > 
> > > > 
> > > > And what is the problem? You're too brief..
> > > 
> > > I guess his point is that if you do
> > > cd a/b/c/
> > > then
> > > cd ..
> > > will not get you back to where you started.  Perhaps you could use
> > > pushd/popd instead.
> > 
> > Or a subshell.
> > 
> > 	Jakub
> > 
> 
> I'm suggesting the following patch.
> 
> Ready to be installed?

LGTM.

> From 77bc388daf42d18334cb874407031fc49dbbaa67 Mon Sep 17 00:00:00 2001
> From: Martin Liska <mliska@suse.cz>
> Date: Fri, 4 Feb 2022 10:24:51 +0100
> Subject: [PATCH] fixincludes: Update pwd.
> 
> fixincludes/ChangeLog:
> 
> 	* fixinc.in: Use cd OLDDIR instead of cd .. .
> ---
>  fixincludes/fixinc.in | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fixincludes/fixinc.in b/fixincludes/fixinc.in
> index 0c3066452c6..0bd8027a554 100755
> --- a/fixincludes/fixinc.in
> +++ b/fixincludes/fixinc.in
> @@ -258,9 +258,10 @@ then echo "All directories (including links to directories):"
>       echo $all_dirs
>  fi
>  
> +OLDDIR=`${PWDCMD}`
>  cd $LIB
>  echo "$all_dirs" | xargs mkdir -p
> -cd ..
> +cd ${OLDDIR}
>  
>  mkdir $LIB/root
>  
> -- 
> 2.35.1
> 


Marek
diff mbox series

Patch

diff --git a/fixincludes/fixinc.in b/fixincludes/fixinc.in
index de5a37f6acc..cdc0ca2e4c4 100755
--- a/fixincludes/fixinc.in
+++ b/fixincludes/fixinc.in
@@ -258,13 +258,9 @@  then echo "All directories (including links to directories):"
       echo $all_dirs
  fi
  
-for file in $all_dirs; do
-  rm -rf $LIB/$file
-  if [ ! -d $LIB/$file ]
-  then mkdir $LIB/$file
-  fi
-done
-mkdir $LIB/root
+cd $LIB
+mkdir -p $all_dirs
+cd ..
  
  # # # # # # # # # # # # # # # # # # # # #
  #