[webkit-changes] [264332] trunk/Source

# Darin Adler (3 days ago)

We need to be cautious about adding header includes to other headers. Adding includes to .cpp files is likely fine and not a deeply consequential decision, but adding to headers is something that can have a massive effect on build times over time.

On Jul 13, 2020, at 10:44 PM, Hironori.Fujii at sony.com wrote: <>Modified: trunk/Source/WebCore/platform/graphics/ColorSerialization.h (264331 => 264332)

--- trunk/Source/WebCore/platform/graphics/ColorSerialization.h 2020-07-14 05:17:20 UTC (rev 264331) +++ trunk/Source/WebCore/platform/graphics/ColorSerialization.h 2020-07-14 05:44:25 UTC (rev 264332) @@ -25,6 +25,8 @@

#pragma once

+#include <wtf/text/WTFString.h>

This change is wrong. The header to include here is Forward.h, not WTFString.h. Not super-harmful since WTFString.h is already so widely included, but we need to be really cautious in headers.

Modified: trunk/Source/WebCore/svg/SVGParserUtilities.h (264331 => 264332)

--- trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:17:20 UTC (rev 264331) +++ trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:44:25 UTC (rev 264332) @@ -24,6 +24,7 @@

#include "ParsingUtilities.h"

#include <wtf/HashSet.h>

#include <wtf/Vector.h> +#include <wtf/text/WTFString.h>

Same mistake.

I’d really like to come up with some other system for reviewing adding headers to headers.

— Darin

Contact us to advertise here
# Said Abou-Hallawa (3 days ago)

On Jul 14, 2020, at 10:40 AM, Darin Adler <darin at apple.com> wrote:

We need to be cautious about adding header includes to other headers. Adding includes to .cpp files is likely fine and not a deeply consequential decision, but adding to headers is something that can have a massive effect on build times over time.

On Jul 13, 2020, at 10:44 PM, Hironori.Fujii at sony.com <mailto:Hironori.Fujii at sony.com> wrote: <>Modified: trunk/Source/WebCore/platform/graphics/ColorSerialization.h (264331 => 264332)

--- trunk/Source/WebCore/platform/graphics/ColorSerialization.h 2020-07-14 05:17:20 UTC (rev 264331) +++ trunk/Source/WebCore/platform/graphics/ColorSerialization.h 2020-07-14 05:44:25 UTC (rev 264332) @@ -25,6 +25,8 @@

#pragma once

+#include <wtf/text/WTFString.h> This change is wrong. The header to include here is Forward.h, not WTFString.h. Not super-harmful since WTFString.h is already so widely included, but we need to be really cautious in headers.

Modified: trunk/Source/WebCore/svg/SVGParserUtilities.h (264331 => 264332)

--- trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:17:20 UTC (rev 264331) +++ trunk/Source/WebCore/svg/SVGParserUtilities.h 2020-07-14 05:44:25 UTC (rev 264332) @@ -24,6 +24,7 @@

#include "ParsingUtilities.h"

#include <wtf/HashSet.h>

#include <wtf/Vector.h> +#include <wtf/text/WTFString.h> Same mistake.

I’d really like to come up with some other system for reviewing adding headers to headers.

I looked at the SVGParserUtilities.h and in fact we do not have to include HashSet.h or Vector.h either. HashSet and Vector need only to be forward declare in this header because they are referenced as return values. So <wtf/Forward.h> can replace all the three header files.

Maybe the policy can be: include <wtf/Forward.h> before trying to include any other wtf header file.

I think we need to include the wtf header files only when the compiler needs to know the size. For example:

class X { Vector<String> m_list; };

Thanks, Said

# Sam Weinig (3 days ago)

While I don’t want to take away from what Darin is saying here about correct usage of forward declaration and <wtf/Forward.h>, I’d like to understand why we have two different compilation models supported in WebKit. Is there a reason both need to be supported? Can we remove one?

# Simon Fraser (3 days ago)

Could someone educate me about <wtf/Forward.h>? When should I use this instead of individual wtf headers?

Simon

# Darin Adler (3 days ago)

On Jul 14, 2020, at 2:38 PM, Simon Fraser <simon.fraser at apple.com> wrote:

Could someone educate me about <wtf/Forward.h>? When should I use this instead of individual wtf headers?

Forward.h is analogous to forward-declaring a class ('class IntPoint;' instead of ‘#include “IntPoint.h”'), but it works for many often-used classes and class templates in the WTF namespace, including class templates that would be difficult to correctly forward-declare due to their many arguments, such as WTF::Vector. And it includes “using WTF::String” and the like, as well, to import WTF namespace things into the global namespace.

We can use it any time we need a forward-declaration, not an entire definition, of one of the items. For example, to compile a header that just takes and returns String objects, we only need a forward declaration of String. The easiest way to correctly do that is to include <wtf/Forward.h>. Including <wtf/WTFString.h> pulls in a lot more. For the specific case of String, I think you might be able to go further and write this instead:

namespace WTF {
class String;
}
using WTF::String;

But I have never tried it, and there might be some problem with that.

— Darin

# Fujii Hironori (3 days ago)

On Wed, Jul 15, 2020 at 6:32 AM Sam Weinig <weinig at apple.com> wrote:

While I don’t want to take away from what Darin is saying here about correct usage of forward declaration and <wtf/Forward.h>, I’d like to understand why we have two different compilation models supported in WebKit. Is there a reason both need to be supported? Can we remove one?

I also want to know that. Does anyone still need non-unified builds?

I introduced other unnecessary header inclusion, and Darin asked me to fix it. bugs.webkit.org/show_bug.cgi?id=214204#c25 Reducing header inclusion can easily cause build breakages for non-unified builds. So, I fixed non-unified build breakage in r264332 and r264333 as the preparation for that. Non-unified builds was more broken than I expected. Does anyone still need it? Should we maintain non-unified builds until C++20 modules will nullify unified builds?

# Alex Christensen (2 days ago)

I think it is still quite useful to fix non-unified builds. Otherwise adding a file usually involves many unrelated build fixes.

# Yusuke Suzuki (2 days ago)

And I agree, keeping non-unified build sane is quite useful. In addition to the benefit described by Alex, this also allows CMake to generate sane compile_commands.json, so that my completion in Neovim works better in cpp files, and I think this compile_commands.json is also used in several clang-related toolings too.

I think we should have a bot maintaining it.

# Geoffrey Garen (a day ago)

The original question was, as I understood it, was do we need to support non-unified builds as an essential requirement for a given port, and if so, why?

I’d like to finish answering that question, before we wonder off-topic and consider whether supporting non-unified builds as an optional way to improve our workflow is a good idea.

Thanks, Geoff

# Kirsling, Ross (18 hours ago)

Non-unified build fixes are done to support all platforms, because sudden unification shifts can happen anywhere. We can't currently perform a full non-unified build on Mac since the CMake build only works up through JSC, so Sony and Igalia have been performing this maintenance by hand on Windows and Linux for years. It is for this reason that one is unlikely to encounter unification shift issues via EWS.

Tips for how to make better build fixes are helpful, but these build fixes can't go anywhere until we have a bot to identify missing includes and such as they arise.

Ross

# Darin Adler (18 hours ago)

On Jul 16, 2020, at 12:54 PM, Kirsling, Ross <Ross.Kirsling at sony.com> wrote:

Non-unified build fixes are done to support all platforms, because sudden unification shifts can happen anywhere.

I’m not sure that benefit is worth the additional code churn. I understand that it’s frustrating to run into a problem when unification shifts, say when you are adding a source file. I believe we have made changes to unification since the earliest version that reduce how often unification shifts.

— Darin

# Adrian Perez de Castro (16 hours ago)

Hello everybody,

On Thu, 16 Jul 2020 12:57:01 -0700, Darin Adler <darin at apple.com> wrote:

On Jul 16, 2020, at 12:54 PM, Kirsling, Ross <Ross.Kirsling at sony.com> wrote:

Non-unified build fixes are done to support all platforms, because sudden unification shifts can happen anywhere.

I’m not sure that benefit is worth the additional code churn. I understand that it’s frustrating to run into a problem when unification shifts, say when you are adding a source file. I believe we have made changes to unification since the earliest version that reduce how often unification shifts.

While this is true, shifts still happen easily when changing anything in the build configuration that affects the list of source files to build. The cases in which I have seen this happening when using the CMake based ports are:

  • The target architecture is not one of the few tested by the bots.

    • For the GTK and WPE ports this means that a build for anything else than x86_64 is susceptible to break easily.

    • Note that JSCOnly builds on ARM, MIPS, and i386 are actively tested by EWS bots, so the JSC build is less likely to.

  • The features enabled at build time are changed from what the EWS bots test.

    • Happens when the build configuration (via CMake) changes the defaults. For example one can build the GTK port with support for Wayland enabled and X11 disabled (the default is both enabled). Or one can build WPE with all media support disabled (which completely avoids needing GStreamer, making the disk footprint considerably smaller: this is sometimes done when targeting embedded devices).

    • Bots test with experimental features enabled, but the default setting when building from source tarballs has those disabled at build time. This is less likely to result in a broken build than manually toggling features, it can happen.

It is not feasible to test unified builds for all the possible combinations of target architectures and set of features enabled at build time (there is a combinatorial explosion of configurations). Keeping non-unified builds in a working state guarantees that regular unified builds will keep working no matter what the build configuration is.

Another side effect of keeping non-unified builds usable is that it is possible to build WebKit on machines with less RAM (granted: only release builds without any debugging information), which sometimes has enabled people who don't have access to beefier machines to get WebKit built and even work on patches. While this has not been very common, I think it's a commendable goal to allow people without access to beefy computers to contribute to WebKit :)

Best

# Adrian Perez de Castro (16 hours ago)

I forgot to mention one tidbit...

On Fri, 17 Jul 2020 00:28:53 +0300, Adrian Perez de Castro <aperez at igalia.com> wrote:

Hello everybody,

On Thu, 16 Jul 2020 12:57:01 -0700, Darin Adler <darin at apple.com> wrote:

On Jul 16, 2020, at 12:54 PM, Kirsling, Ross <Ross.Kirsling at sony.com> wrote:

Non-unified build fixes are done to support all platforms, because sudden unification shifts can happen anywhere.

I’m not sure that benefit is worth the additional code churn. I understand that it’s frustrating to run into a problem when unification shifts, say when you are adding a source file. I believe we have made changes to unification since the earliest version that reduce how often unification shifts.

While this is true, shifts still happen easily when changing anything in the build configuration that affects the list of source files to build. The cases in which I have seen this happening when using the CMake based ports are:

  • The target architecture is not one of the few tested by the bots.

    • For the GTK and WPE ports this means that a build for anything else than x86_64 is susceptible to break easily.

    • Note that JSCOnly builds on ARM, MIPS, and i386 are actively tested by EWS bots, so the JSC build is less likely to.

  • The features enabled at build time are changed from what the EWS bots test.

    • Happens when the build configuration (via CMake) changes the defaults. For example one can build the GTK port with support for Wayland enabled and X11 disabled (the default is both enabled). Or one can build WPE with all media support disabled (which completely avoids needing GStreamer, making the disk footprint considerably smaller: this is sometimes done when targeting embedded devices).

As a matter of fact: lately I have been trying to make sure that non-unified builds are working right before making source tarball releases for the WPE port, and the reports of build failures due to unification issues has been zero ever since.

Also, some packagers used to carry assorted downstream patches for build issues related to unification build which have not been needed anymore. The case I know better is Buildroot [1]: when I started maintaining its package for the GTK port in preparation to submit the WPE package one of the tasks I did was precisely avoid downstream patches, by applying the fixes to WebKit itself—and that was when I learnt that making non-unified builds work was a good thing.

  • Bots test with experimental features enabled, but the default setting when building from source tarballs has those disabled at build time. This is less likely to result in a broken build than manually toggling features, it can happen.

It is not feasible to test unified builds for all the possible combinations of target architectures and set of features enabled at build time (there is a combinatorial explosion of configurations). Keeping non-unified builds in a working state guarantees that regular unified builds will keep working no matter what the build configuration is.

Another side effect of keeping non-unified builds usable is that it is possible to build WebKit on machines with less RAM (granted: only release builds without any debugging information), which sometimes has enabled people who don't have access to beefier machines to get WebKit built and even work on patches. While this has not been very common, I think it's a commendable goal to allow people without access to beefy computers to contribute to WebKit :)

Best regards, —Adrián


[1] buildroot.org

# Fujii Hironori (16 hours ago)

I'm glad to hear various opinions. Slack still can't beat mailing lists for technical discussions.

On Fri, Jul 17, 2020 at 6:37 AM Adrian Perez de Castro <aperez at igalia.com>

wrote:

Also, some packagers used to carry assorted downstream patches for build issues related to unification build which have not been needed anymore.

Unified source builds can have performance merit. The SQLite Amalgamation makes it 5% and 10% faster. www.sqlite.org/amalgamation.html

# Darin Adler (16 hours ago)

On Jul 16, 2020, at 2:28 PM, Adrian Perez de Castro <aperez at igalia.com> wrote:

It is not feasible to test unified builds for all the possible combinations of target architectures and set of features enabled at build time (there is a combinatorial explosion of configurations). Keeping non-unified builds in a working state guarantees that regular unified builds will keep working no matter what the build configuration is.

But it doesn’t.

Non-unified builds don’t make it possible to have arbitrary sets of features enabled at build time. We break that all the time and it’s only fixed when someone tests that arbitrary set of features. Headers are only a small part of that issue.

I’d go as far as saying it’s not a project goal to always build with arbitrary sets of features enabled.

— Darin

# Adrian Perez de Castro (15 hours ago)

On Fri, 17 Jul 2020 06:52:54 +0900, Fujii Hironori <fujii.hironori at gmail.com> wrote:

I'm glad to hear various opinions. Slack still can't beat mailing lists for technical discussions.

Aye, in my experience mailing lists are better because having to write down thoughts in a slightly more structured way (in a mail message to be read from start to end in one go) results in people organizing their thoughts better (as opposed to a more or less random exchange of loosely organized chat messages).

Not to mention that using a mailing list also records discussions and the decisions, thanks to the messages being archived, which is more “durable” for future reference. Oh, and everybody can easily grab a copy of the mailing list archives (from the Mailman web interface as mboxes, or keeping their local copy) so it's less likely that all copies of mailing list archives would suddenly disappear; but OTOH, as unlikely as it may seem, Slack could go bankrupt and/or the chat history vanish from one day to another.

On Fri, Jul 17, 2020 at 6:37 AM Adrian Perez de Castro <aperez at igalia.com> wrote:

Also, some packagers used to carry assorted downstream patches for build issues related to unification build which have not been needed anymore.

Unified source builds can have performance merit. The SQLite Amalgamation makes it 5% and 10% faster. www.sqlite.org/amalgamation.html

Well, this is what Link-Time Optimization is for, without needing to resort to ugly preprocessor tricks. I would rather let the compiler and linker do their job—it's 2020 and any serious toolchain supports it by now ;-)

# Adrian Perez de Castro (15 hours ago)

On Thu, 16 Jul 2020 15:01:14 -0700, Darin Adler <darin at apple.com> wrote:

On Jul 16, 2020, at 2:28 PM, Adrian Perez de Castro <aperez at igalia.com> wrote:

It is not feasible to test unified builds for all the possible combinations of target architectures and set of features enabled at build time (there is a combinatorial explosion of configurations). Keeping non-unified builds in a working state guarantees that regular unified builds will keep working no matter what the build configuration is.

But it doesn’t.

Yes, because enabling/disabling features changes the set of source files to build, which results in different source files being #included from each unifier source compilation unit.

Non-unified builds don’t make it possible to have arbitrary sets of features enabled at build time. We break that all the time and it’s only fixed when someone tests that arbitrary set of features. Headers are only a small part of that issue.

True, and that is why our message for the GTK and WPE ports is always that we only support builds with the default build options, but of course anyone is free to change them their mileage may vary.

In practice, we tend to keep things working in a best-effort basis for the following few cases:

  • Disabling the Wayland or X11 support in the GTK port.
  • Disabling the WPE renderer in the GTK port.
  • Disabling multimedia support in the WPE port.

I’d go as far as saying it’s not a project goal to always build with arbitrary sets of features enabled.

I agree. But even toggling the few features I mentioned above, or the target architecture to something else than what the bots use, results in build failures related to unified builds due to different sources being #included in each unified compilation unit ¯_(ツ)_/¯

# Darin Adler (14 hours ago)

On Jul 16, 2020, at 3:33 PM, Adrian Perez de Castro <aperez at igalia.com> wrote:

enabling/disabling features changes the set of source files to build, which results in different source files being #included from each unifier source compilation unit.

Let’s stop doing it that way. Just compile the files and have #if inside the file.

I removed all conditional source files from the Xcode build system. Let's do the same for the CMake build system.

— Darin

# Michael Catanzaro (13 hours ago)

On Thu, Jul 16, 2020 at 4:14 pm, Darin Adler <darin at apple.com> wrote:

Let’s stop doing it that way. Just compile the files and have #if inside the file.

I removed all conditional source files from the Xcode build system. Let's do the same for the CMake build system.

— Darin

I agree we should do this. We've never been consistent between whether we guard files at the build system level or inside the files themselves. But with unified builds, the advantage of putting the guards inside the files is clear: we would ensure that the set of files compiled is always the same for a given port regardless of which options are used.

I wouldn't use that as a reason to drop non-unified builds, though. E.g. Yusuke's argument regarding compile_commands.json seems pretty compelling.

Want more features?

Request early access to our private beta of readable email premium.