diff mbox series

[1/1] package/valgrind: Add a setting for LTO build

Message ID 20210418183658.1675843-1-glex.spb@gmail.com
State Rejected
Headers show
Series [1/1] package/valgrind: Add a setting for LTO build | expand

Commit Message

Gleb Mazovetskiy April 18, 2021, 6:36 p.m. UTC
Valgrind LTO build takes over an hour on my Ryzen 3950x.

This is not always worth it for the ~10% size reduction and a small
performance improvement it offers (see https://www.valgrind.org/docs/manual/dist.news.html)

Adds a new `BR2_VALGRIND_ENABLE_LTO` setting to control LTO enablement
for Valgrind separately from the global BR2_GCC_ENABLE_LTO setting.

Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
---
 package/valgrind/Config.in   | 10 ++++++++++
 package/valgrind/valgrind.mk |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

Peter Seiderer April 18, 2021, 9:59 p.m. UTC | #1
Hello Gleb,
On Sun, 18 Apr 2021 19:36:58 +0100, Gleb Mazovetskiy <glex.spb@gmail.com> wrote:

> Valgrind LTO build takes over an hour on my Ryzen 3950x.
>

Can confirm a build time increase (AMD Ryzen 7 1800X):

- without LTO:

real	0m54,741s
user	3m29,941s
sys	0m10,949s

- with LTO:

real	5m48,009s
user	6m23,040s
sys	0m13,197s

> This is not always worth it for the ~10% size reduction and a small
> performance improvement it offers (see https://www.valgrind.org/docs/manual/dist.news.html)
>
> Adds a new `BR2_VALGRIND_ENABLE_LTO` setting to control LTO enablement
> for Valgrind separately from the global BR2_GCC_ENABLE_LTO setting.
>
> Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
> ---
>  package/valgrind/Config.in   | 10 ++++++++++
>  package/valgrind/valgrind.mk |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/package/valgrind/Config.in b/package/valgrind/Config.in
> index 56e4af21fa..eff54e7949 100644
> --- a/package/valgrind/Config.in
> +++ b/package/valgrind/Config.in
> @@ -84,4 +84,14 @@ config BR2_PACKAGE_VALGRIND_NULGRIND
>  	help
>  	  This option allows to install the Nulgrind tool
>
> +if BR2_GCC_ENABLE_LTO
> +
> +config BR2_VALGRIND_ENABLE_LTO
> +	bool "Build Valgrind with link-time optimization"
> +	help
> +	  This produces a faster / smaller valgrind (up to 10%) at the cost
> +	  of massively increased build time (can be over an hour).

Be careful with absolute times, depends strongly on the used hardware...
(out of interest, what is your baseline compile time?)...

Would move this section up-to/before the tools selection section...

Regards,
Peter

> +
> +endif
> +
>  endif
> diff --git a/package/valgrind/valgrind.mk b/package/valgrind/valgrind.mk
> index b87e1e518f..07fb1f7d42 100644
> --- a/package/valgrind/valgrind.mk
> +++ b/package/valgrind/valgrind.mk
> @@ -50,7 +50,7 @@ VALGRIND_CONF_OPTS += \
>  	--host=$(patsubst arm-%,armv7-%,$(GNU_TARGET_NAME))
>  endif
>
> -ifeq ($(BR2_GCC_ENABLE_LTO),y)
> +ifeq ($(BR2_VALGRIND_ENABLE_LTO),y)
>  VALGRIND_CONF_OPTS += --enable-lto
>  else
>  VALGRIND_CONF_OPTS += --disable-lto
Gleb Mazovetskiy April 19, 2021, 2:01 a.m. UTC | #2
By the way, I should've mentioned that it's over 1 hour for MIPS32R2 with
uclibc


On Sun, Apr 18, 2021 at 10:59 PM Peter Seiderer <ps.report@gmx.net> wrote:

> Hello Gleb,
> On Sun, 18 Apr 2021 19:36:58 +0100, Gleb Mazovetskiy <glex.spb@gmail.com>
> wrote:
>
> > Valgrind LTO build takes over an hour on my Ryzen 3950x.
> >
>
> Can confirm a build time increase (AMD Ryzen 7 1800X):
>
> - without LTO:
>
> real    0m54,741s
> user    3m29,941s
> sys     0m10,949s
>
> - with LTO:
>
> real    5m48,009s
> user    6m23,040s
> sys     0m13,197s
>
> > This is not always worth it for the ~10% size reduction and a small
> > performance improvement it offers (see
> https://www.valgrind.org/docs/manual/dist.news.html)
> >
> > Adds a new `BR2_VALGRIND_ENABLE_LTO` setting to control LTO enablement
> > for Valgrind separately from the global BR2_GCC_ENABLE_LTO setting.
> >
> > Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
> > ---
> >  package/valgrind/Config.in   | 10 ++++++++++
> >  package/valgrind/valgrind.mk |  2 +-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/package/valgrind/Config.in b/package/valgrind/Config.in
> > index 56e4af21fa..eff54e7949 100644
> > --- a/package/valgrind/Config.in
> > +++ b/package/valgrind/Config.in
> > @@ -84,4 +84,14 @@ config BR2_PACKAGE_VALGRIND_NULGRIND
> >       help
> >         This option allows to install the Nulgrind tool
> >
> > +if BR2_GCC_ENABLE_LTO
> > +
> > +config BR2_VALGRIND_ENABLE_LTO
> > +     bool "Build Valgrind with link-time optimization"
> > +     help
> > +       This produces a faster / smaller valgrind (up to 10%) at the cost
> > +       of massively increased build time (can be over an hour).
>
> Be careful with absolute times, depends strongly on the used hardware...
> (out of interest, what is your baseline compile time?)...
>
> Would move this section up-to/before the tools selection section...
>
> Regards,
> Peter
>
> > +
> > +endif
> > +
> >  endif
> > diff --git a/package/valgrind/valgrind.mk b/package/valgrind/valgrind.mk
> > index b87e1e518f..07fb1f7d42 100644
> > --- a/package/valgrind/valgrind.mk
> > +++ b/package/valgrind/valgrind.mk
> > @@ -50,7 +50,7 @@ VALGRIND_CONF_OPTS += \
> >       --host=$(patsubst arm-%,armv7-%,$(GNU_TARGET_NAME))
> >  endif
> >
> > -ifeq ($(BR2_GCC_ENABLE_LTO),y)
> > +ifeq ($(BR2_VALGRIND_ENABLE_LTO),y)
> >  VALGRIND_CONF_OPTS += --enable-lto
> >  else
> >  VALGRIND_CONF_OPTS += --disable-lto
>
>
Arnout Vandecappelle July 25, 2022, 3:27 p.m. UTC | #3
On 18/04/2021 23:59, Peter Seiderer wrote:
> Hello Gleb,
> On Sun, 18 Apr 2021 19:36:58 +0100, Gleb Mazovetskiy <glex.spb@gmail.com> wrote:
> 
>> Valgrind LTO build takes over an hour on my Ryzen 3950x.
>>
> 
> Can confirm a build time increase (AMD Ryzen 7 1800X):
> 
> - without LTO:
> 
> real	0m54,741s
> user	3m29,941s
> sys	0m10,949s
> 
> - with LTO:
> 
> real	5m48,009s
> user	6m23,040s
> sys	0m13,197s
> 
>> This is not always worth it for the ~10% size reduction and a small
>> performance improvement it offers (see https://www.valgrind.org/docs/manual/dist.news.html)
>>
>> Adds a new `BR2_VALGRIND_ENABLE_LTO` setting to control LTO enablement
>> for Valgrind separately from the global BR2_GCC_ENABLE_LTO setting.

  Adding per-package options to enable debug and optimisation features is really 
not the way to go.

  Instead what I've done is to create a series [1] that turns the LTO option in 
a global option, that clearly explains in the help text that it may have a big 
impact on build time. I've removed the BR2_GCC_ENABLE_LTO option entirely 
because it was not actually doing what it advertises.

  Let me know what you think.

  I've marked this patch as Rejected.

  Regards,
  Arnout

[1] https://patchwork.ozlabs.org/project/buildroot/list/?series=311175

>>
>> Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
>> ---
>>   package/valgrind/Config.in   | 10 ++++++++++
>>   package/valgrind/valgrind.mk |  2 +-
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/package/valgrind/Config.in b/package/valgrind/Config.in
>> index 56e4af21fa..eff54e7949 100644
>> --- a/package/valgrind/Config.in
>> +++ b/package/valgrind/Config.in
>> @@ -84,4 +84,14 @@ config BR2_PACKAGE_VALGRIND_NULGRIND
>>   	help
>>   	  This option allows to install the Nulgrind tool
>>
>> +if BR2_GCC_ENABLE_LTO
>> +
>> +config BR2_VALGRIND_ENABLE_LTO
>> +	bool "Build Valgrind with link-time optimization"
>> +	help
>> +	  This produces a faster / smaller valgrind (up to 10%) at the cost
>> +	  of massively increased build time (can be over an hour).
> 
> Be careful with absolute times, depends strongly on the used hardware...
> (out of interest, what is your baseline compile time?)...
> 
> Would move this section up-to/before the tools selection section...
> 
> Regards,
> Peter
> 
>> +
>> +endif
>> +
>>   endif
>> diff --git a/package/valgrind/valgrind.mk b/package/valgrind/valgrind.mk
>> index b87e1e518f..07fb1f7d42 100644
>> --- a/package/valgrind/valgrind.mk
>> +++ b/package/valgrind/valgrind.mk
>> @@ -50,7 +50,7 @@ VALGRIND_CONF_OPTS += \
>>   	--host=$(patsubst arm-%,armv7-%,$(GNU_TARGET_NAME))
>>   endif
>>
>> -ifeq ($(BR2_GCC_ENABLE_LTO),y)
>> +ifeq ($(BR2_VALGRIND_ENABLE_LTO),y)
>>   VALGRIND_CONF_OPTS += --enable-lto
>>   else
>>   VALGRIND_CONF_OPTS += --disable-lto
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Gleb Mazovetskiy July 25, 2022, 6 p.m. UTC | #4
We don't want to disable LTO globally only for certain packages We want LTO
generally but not for a development tool that takes over 1 hour to build
with LTO vs 1 minute without it.

Is it possible to do it on a per-package basis?

By the way, it would also be useful to control BR2_STATIC_LIBS on a
per-package basis (recent example:
https://patchwork.ozlabs.org/project/buildroot/patch/20220620115510.33792-1-paul@crapouillou.net/
)

On Mon, Jul 25, 2022 at 4:27 PM Arnout Vandecappelle <arnout@mind.be> wrote:

>
>
> On 18/04/2021 23:59, Peter Seiderer wrote:
> > Hello Gleb,
> > On Sun, 18 Apr 2021 19:36:58 +0100, Gleb Mazovetskiy <glex.spb@gmail.com>
> wrote:
> >
> >> Valgrind LTO build takes over an hour on my Ryzen 3950x.
> >>
> >
> > Can confirm a build time increase (AMD Ryzen 7 1800X):
> >
> > - without LTO:
> >
> > real  0m54,741s
> > user  3m29,941s
> > sys   0m10,949s
> >
> > - with LTO:
> >
> > real  5m48,009s
> > user  6m23,040s
> > sys   0m13,197s
> >
> >> This is not always worth it for the ~10% size reduction and a small
> >> performance improvement it offers (see
> https://www.valgrind.org/docs/manual/dist.news.html)
> >>
> >> Adds a new `BR2_VALGRIND_ENABLE_LTO` setting to control LTO enablement
> >> for Valgrind separately from the global BR2_GCC_ENABLE_LTO setting.
>
>   Adding per-package options to enable debug and optimisation features is
> really
> not the way to go.
>
>   Instead what I've done is to create a series [1] that turns the LTO
> option in
> a global option, that clearly explains in the help text that it may have a
> big
> impact on build time. I've removed the BR2_GCC_ENABLE_LTO option entirely
> because it was not actually doing what it advertises.
>
>   Let me know what you think.
>
>   I've marked this patch as Rejected.
>
>   Regards,
>   Arnout
>
> [1] https://patchwork.ozlabs.org/project/buildroot/list/?series=311175
>
> >>
> >> Signed-off-by: Gleb Mazovetskiy <glex.spb@gmail.com>
> >> ---
> >>   package/valgrind/Config.in   | 10 ++++++++++
> >>   package/valgrind/valgrind.mk |  2 +-
> >>   2 files changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/package/valgrind/Config.in b/package/valgrind/Config.in
> >> index 56e4af21fa..eff54e7949 100644
> >> --- a/package/valgrind/Config.in
> >> +++ b/package/valgrind/Config.in
> >> @@ -84,4 +84,14 @@ config BR2_PACKAGE_VALGRIND_NULGRIND
> >>      help
> >>        This option allows to install the Nulgrind tool
> >>
> >> +if BR2_GCC_ENABLE_LTO
> >> +
> >> +config BR2_VALGRIND_ENABLE_LTO
> >> +    bool "Build Valgrind with link-time optimization"
> >> +    help
> >> +      This produces a faster / smaller valgrind (up to 10%) at the cost
> >> +      of massively increased build time (can be over an hour).
> >
> > Be careful with absolute times, depends strongly on the used hardware...
> > (out of interest, what is your baseline compile time?)...
> >
> > Would move this section up-to/before the tools selection section...
> >
> > Regards,
> > Peter
> >
> >> +
> >> +endif
> >> +
> >>   endif
> >> diff --git a/package/valgrind/valgrind.mk b/package/valgrind/
> valgrind.mk
> >> index b87e1e518f..07fb1f7d42 100644
> >> --- a/package/valgrind/valgrind.mk
> >> +++ b/package/valgrind/valgrind.mk
> >> @@ -50,7 +50,7 @@ VALGRIND_CONF_OPTS += \
> >>      --host=$(patsubst arm-%,armv7-%,$(GNU_TARGET_NAME))
> >>   endif
> >>
> >> -ifeq ($(BR2_GCC_ENABLE_LTO),y)
> >> +ifeq ($(BR2_VALGRIND_ENABLE_LTO),y)
> >>   VALGRIND_CONF_OPTS += --enable-lto
> >>   else
> >>   VALGRIND_CONF_OPTS += --disable-lto
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot@busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot
>
diff mbox series

Patch

diff --git a/package/valgrind/Config.in b/package/valgrind/Config.in
index 56e4af21fa..eff54e7949 100644
--- a/package/valgrind/Config.in
+++ b/package/valgrind/Config.in
@@ -84,4 +84,14 @@  config BR2_PACKAGE_VALGRIND_NULGRIND
 	help
 	  This option allows to install the Nulgrind tool
 
+if BR2_GCC_ENABLE_LTO
+
+config BR2_VALGRIND_ENABLE_LTO
+	bool "Build Valgrind with link-time optimization"
+	help
+	  This produces a faster / smaller valgrind (up to 10%) at the cost
+	  of massively increased build time (can be over an hour).
+
+endif
+
 endif
diff --git a/package/valgrind/valgrind.mk b/package/valgrind/valgrind.mk
index b87e1e518f..07fb1f7d42 100644
--- a/package/valgrind/valgrind.mk
+++ b/package/valgrind/valgrind.mk
@@ -50,7 +50,7 @@  VALGRIND_CONF_OPTS += \
 	--host=$(patsubst arm-%,armv7-%,$(GNU_TARGET_NAME))
 endif
 
-ifeq ($(BR2_GCC_ENABLE_LTO),y)
+ifeq ($(BR2_VALGRIND_ENABLE_LTO),y)
 VALGRIND_CONF_OPTS += --enable-lto
 else
 VALGRIND_CONF_OPTS += --disable-lto