Bug 235641 – [CMake] Pass -pipe to compilers that support it

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adrian Perez]

Adding -pipe to the compiler command makes it use a pipe to send its
output to the assembler instead of temporary files, resulting in faster
builds.

The following results show that fresh builds took 12m:27s less on
average across the five builds I did for testing:

  without -pipe: 35m:43s
     with -pipe: 23m:15s

The setup was:

 - GTK port, release build.
 - Flatpak SDK, GCC.
 - Unified sources (the default).
 - No previous build directory (“rm -rf WebKitBuild/GTK”).
 - ccache empty (“webkit-flatpak -c ccache -cC”).
 - Kernel disk caches flushed (“echo 3 > /proc/sys/vm/drop_caches”).

The GCC documentation warns that some oddball assemblers might not
properly handle input from a pipe, but I would expect that reasonably
modern systems as needed to build/run WebKit will not have that kind
of trouble.

Clang also supports the option, I only checked that the build didn't
get broken but didn't do the numbers — let me know if you want me to
before landing the patch.

[Adrian Perez]

Created attachment 450016 [details]
Patch

[Alicia Boya García]

Comment on attachment 450016 [details]
Patch

I would add the rationale about the performance improvement that is in the bugzilla issue to the ChangeLog.

[Michael Catanzaro]

So in this case, it doesn't make any difference because there is no -no-pipe flag that you can pass to override -pipe, but in general we want to PREPEND -- not APPEND -- any flag that is not strictly required to build WebKit, so that it can be overridden at build time if desired. -fno-exceptions and -fno-rtti and -fno-strict-aliasing are special because you will get a broken WebKit if you don't use these flags. All distros build with -fexceptions (it's required for call stack hardening in C) and they'll get a busted WebKit if we don't override that. Again, I know it doesn't make any difference in this particular case, but emphasizing that these flags are special and that everything else should use PREPEND makes sense to me. There are already comments to warn about this, but they're at the top of the file, far from point of use, so you probably didn't notice.

The other thing is: we probably want to treat this flag the same way we would treat something like -flto=auto or hardening flags like -fstack-clash-protection: you really always want them, but all distros expect that they control these flags and it's redundant (or at worst, conflicting) if projects mess with them. But anybody building WebKit manually will never set these flags, which is sad. I think it makes sense to either have one option for enabling extra recommended compiler flags, or else one option to *disable* extra recommended compiler flags. But -pipe should surely be treated as one of those, not bunched in with our current choices of flags. Basically, we currently do not set any flags that distros are likely to set themselves, and -pipe would be the first such flag. So let's segregate them from our other build options and gate them behind something new.

Counterargument to above proposal: it's not easy to set secure/recommended compiler flags from a CMake build system because it requires writing GCC spec files; therefore, it might be hard to take this farther than just -pipe. For freedesktop-sdk-based stuff like gnome-build-meta, we can hardcode compiler flags easily enough:

https://gitlab.gnome.org/GNOME/gnome-build-meta/-/blob/f0de8a75c49f26f16910992f22e49dc8641a71c2/project.conf#L290

but that depends on special knowledge about nonstandard flags used to control GCC's hardening behavior:

https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/blob/f5598974a078b42ff55098d97b38c85cf2826875/elements/bootstrap/gcc.bst#L66

And that won't work for WebKit because we don't control how GCC is built. So we must instead use complicated GCC specs to get a recommendable set of flags, like:

https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/redhat-hardened-cc1
https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/rawhide/f/redhat-hardened-ld

This makes it tough to set recommendable hardening flags from WebKit. And -pipe is one such flag, so maybe we should give up instead of using it? Adrian, what do you think? I suppose we could treat -pipe differently from other recommendable flags because it is so simple and should have no impact on the output of the build, just make it a little faster and save some I/O. But I don't like inconsistency. :P We have many different ways we could choose to handle this.

Lastly: I just read Alicia's comment, alluding to a noticeable performance improvement. If it really makes a significant difference, then we should of course use it. Maybe segregate it to a different portion of the file, though? Depends on whether you plan to add other recommendable flags in the future or not.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> This makes it tough to set recommendable hardening flags from WebKit. And
> -pipe is one such flag,

Eh, of course I meant: "this makes it tough to set recommendable flags from WebKit" (of course -pipe is not a hardening flag, but the most important recommendable flags are the hardening flags, so offering an option to enable recommendable flags without figuring this out doesn't make sense).

I suppose you were trying to dodge all those concerns by starting with -pipe and nothing else. Which is OK.

[Adrian Perez]

I forgot to mention that the build machine used for testing has a tmpfs
mounted at “/tmp”, which makes the speedup even more impressive:

  % grep /tmp /proc/mounts       
  tmpfs /tmp tmpfs rw,nosuid,nodev,size=65984084k,nr_inodes=409600,inode64 0 0

While I do not plan to spend time figuring out exactly *why* the speedup
is so visible, a couple of theories involve inheriting a file descriptors
being faster than having both compiler and assembler do more syscalls
(open+close syscalls, possibly stat, and traversing file system i-nodes,
checking credentials and mode bits, etc), and the kernel can move data
more efficiently from one process to another with a pipe (rather than
doing more memory copies).

[Adrian Perez]

(In reply to Michael Catanzaro from comment #3)

> [...]
> 
> This makes it tough to set recommendable hardening flags from WebKit. And
> -pipe is one such flag, so maybe we should give up instead of using it?
> Adrian, what do you think? I suppose we could treat -pipe differently from
> other recommendable flags because it is so simple and should have no impact
> on the output of the build, just make it a little faster and save some I/O.
> But I don't like inconsistency. :P We have many different ways we could
> choose to handle this.

While I agree with the sentiment of having some way of telling CMake “please
add recommended compiler options” or “leave it to me, I'll take care of adding
them” for things which do affect the generated compiler output (like hardening
options), I disagree with giving “-pipe” the same treatment, for three reasons:

 - The “-pipe” option does not change at all the generated output, it only
   makes the compiler frontend pass data from one process (compiler) to
   another (assembler) differently.

 - The fact that the “-pipe” option exists at all is completely absurd. The
   compiler frontend should be able to determine whether using pipes is
   possible and enable it automatically by itself.

 - Even if we entertain the idea that having a “-pipe” option is needed (let's
   say, for example, that the compiler frontend cannot ever reliably know whether
   to enable it), it still should be the default, given that e.g. GCC is designed
   to be used along with the GNU Assembler which *does* support using pipes. The
   exception are assemblers that choke on pipes, hence the option should have
   been “-no-pipe” 🙃️

Therefore, I say we should always add “-pipe” and if someone ever finds a toolchain
where it won't work they need to either fix their toolchain or get a decent one.
There is no excuse to have a bad assembler when there are excellent, portable,
open source options.

For other options which *do* affect the generated compiler output, let's open a
different bug.

[Michael Catanzaro]

(In reply to Adrian Perez from comment #0)
> The following results show that fresh builds took 12m:27s less on
> average across the five builds I did for testing:
> 
>   without -pipe: 35m:43s
>      with -pipe: 23m:15s

Oh wow. It seems I just jumped straight to patch review without reading your comment at all. Wowww. Yeah OK, -pipe seems good. I would still use PREPEND rather than APPEND, and consider whether it makes sense to move it to a new section in WebKitCompilerFlags.cmake rather than inserting it where you have it now.

> I forgot to mention that the build machine used for testing has a tmpfs
mounted at “/tmp”, which makes the speedup even more impressive:

This has been default in Fedora and Arch for something around one decade now. I'm amazed that some distros still put /tmp on disk. :/ However, since /tmp on tmpfs would make use of temporary files a lot faster, I would expect it would actually reduce the benefit of using -pipe, not increase it.

> Clang also supports the option, I only checked that the build didn't
> get broken but didn't do the numbers — let me know if you want me to
> before landing the patch.

It's certainly not necessary. Every distro uses -pipe for every build with both GCC and Clang. It's standard and has no downsides (other than the oddball assembler problem). That said, it's used as a "maybe make things a little faster" flag, not a "lol this should speed up the build by 50%" flag. That's just astounding....

[Michael Catanzaro]

(In reply to Adrian Perez from comment #6)
> While I agree with the sentiment of having some way of telling CMake “please
> add recommended compiler options” or “leave it to me, I'll take care of
> adding
> them” for things which do affect the generated compiler output (like
> hardening
> options), I disagree with giving “-pipe” the same treatment, for three
> reasons:

Hmmm, OK, your arguments for lumping it in with the other build options make sense to me. I'd still switch it to PREPEND, though, for consistency (yes, even though it won't make any difference).

[Michael Catanzaro]

Out of curiosity: does your system have a spinning metal disk? Or somehow otherwise have terrible I/O performance, like a virtual private server with remote storage rather than an attached disk?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #9)
> Out of curiosity: does your system have a spinning metal disk? Or somehow
> otherwise have terrible I/O performance, like a virtual private server with
> remote storage rather than an attached disk?

This is the build machine I use, with a relatively decent-but-not-too-new
Samsung NVMe SSD. We have had a bit of a discussion about “-pipe” with
some colleagues from work and turns out building a few thousand times
a simple hello-world program with/witout the option results in measurable
build time differences.

I will move the addition around and use PREPEND for the option, then
submit a new version of the patch =)

[Adrian Perez]

Created attachment 450023 [details]
Patch v2

[EWS]

Committed r288624 (246443@main): <https://commits.webkit.org/246443@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450023 [details].

[Radar WebKit Bug Importer]

<rdar://problem/88080910>

[Michael Catanzaro]

Hm, toolchain devs warn:

"requires substantially more RAM, IIRC, than the non-pipe mode"

I wonder what the impact on RAM is. It won't be a big deal for most WebKit developers, but normal folks building WebKit have enough trouble as it is. Worst case, if it does make a big difference, then we might gate it behind DEVELOPER_MODE on the understanding that people using DEVELOPER_MODE probably have lots and lots of RAM.