[PATCH v2] kbuild: remove sed command from cmd_ar_builtin

Masahiro Yamada posted 1 patch 3 years, 10 months ago
scripts/Makefile.build | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH v2] kbuild: remove sed command from cmd_ar_builtin
Posted by Masahiro Yamada 3 years, 10 months ago
Replace a pipeline of echo and sed with printf to decrease process forks.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

Changes in v2:
  - Avoid the pipeline if there is no object to put in the archive

 scripts/Makefile.build | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index cac070aee791..784f46d41959 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -358,9 +358,8 @@ $(subdir-modorder): $(obj)/%/modules.order: $(obj)/% ;
 
 quiet_cmd_ar_builtin = AR      $@
       cmd_ar_builtin = rm -f $@; \
-		echo $(patsubst $(obj)/%,%,$(real-prereqs)) | \
-		sed -E 's:([^ ]+):$(obj)/\1:g' | \
-		xargs $(AR) cDPrST $@
+	$(if $(real-prereqs), printf "$(obj)/%s " $(patsubst $(obj)/%,%,$(real-prereqs)) | xargs) \
+	$(AR) cDPrST $@
 
 $(obj)/built-in.a: $(real-obj-y) FORCE
 	$(call if_changed,ar_builtin)
-- 
2.32.0
Re: [PATCH v2] kbuild: remove sed command from cmd_ar_builtin
Posted by Nick Desaulniers 3 years, 10 months ago
On Mon, Jun 13, 2022 at 10:53 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Replace a pipeline of echo and sed with printf to decrease process forks.

If you're trying to minimize process forks, is it possible to remove
the use of xargs as well and just invoke $(AR) with the parameters
splatted out? I don't know myself, but maybe you're creative enough?

Otherwise,
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
> Changes in v2:
>   - Avoid the pipeline if there is no object to put in the archive
>
>  scripts/Makefile.build | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index cac070aee791..784f46d41959 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -358,9 +358,8 @@ $(subdir-modorder): $(obj)/%/modules.order: $(obj)/% ;
>
>  quiet_cmd_ar_builtin = AR      $@
>        cmd_ar_builtin = rm -f $@; \
> -               echo $(patsubst $(obj)/%,%,$(real-prereqs)) | \
> -               sed -E 's:([^ ]+):$(obj)/\1:g' | \
> -               xargs $(AR) cDPrST $@
> +       $(if $(real-prereqs), printf "$(obj)/%s " $(patsubst $(obj)/%,%,$(real-prereqs)) | xargs) \
> +       $(AR) cDPrST $@
>
>  $(obj)/built-in.a: $(real-obj-y) FORCE
>         $(call if_changed,ar_builtin)
> --
> 2.32.0
>


-- 
Thanks,
~Nick Desaulniers
Re: [PATCH v2] kbuild: remove sed command from cmd_ar_builtin
Posted by Masahiro Yamada 3 years, 10 months ago
On Wed, Jun 15, 2022 at 3:59 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Mon, Jun 13, 2022 at 10:53 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > Replace a pipeline of echo and sed with printf to decrease process forks.
>
> If you're trying to minimize process forks, is it possible to remove
> the use of xargs as well and just invoke $(AR) with the parameters
> splatted out? I don't know myself, but maybe you're creative enough?


If I remove xargs, we will go back to the situation
before cd968b97c49214e6557381bddddacbd0e0fb696e.

This patch tries to avoid "too long argument error"
without forking too many processes.
Maybe I am too worried about the potential issue, though...






-- 
Best Regards
Masahiro Yamada
Re: [PATCH v2] kbuild: remove sed command from cmd_ar_builtin
Posted by Jeff Johnson 3 years, 10 months ago
On 6/14/2022 8:03 PM, Masahiro Yamada wrote:
> On Wed, Jun 15, 2022 at 3:59 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
>>
>> On Mon, Jun 13, 2022 at 10:53 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>>>
>>> Replace a pipeline of echo and sed with printf to decrease process forks.
>>
>> If you're trying to minimize process forks, is it possible to remove
>> the use of xargs as well and just invoke $(AR) with the parameters
>> splatted out? I don't know myself, but maybe you're creative enough?
> 
> 
> If I remove xargs, we will go back to the situation
> before cd968b97c49214e6557381bddddacbd0e0fb696e.
> 
> This patch tries to avoid "too long argument error"
> without forking too many processes.
> Maybe I am too worried about the potential issue, though...

 From my very clouded perspective avoiding "too long argument error" 
should take priority :)