[webkit-dev] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)

[Michael Catanzaro]

I'd like to propose a variation on Keith's proposal. I think we should 
manually determine which files should be built together, rather than 
writing CMake code to do that automatically.

Let's consider an example of what use of the FILENAME namespace, as 
Keith proposes, might look like. I opened a couple big files in WebCore 
and settled on this representative code snippet from 
HTMLMediaElement.cpp:

> #if ENABLE(MEDIA_SESSION)
> typedef HashMap<uint64_t, HTMLMediaElement*> IDToElementMap;
> 
> static IDToElementMap& elementIDsToElements()
> {
>     static NeverDestroyed<IDToElementMap> map;
>     return map;
> }
> 
> HTMLMediaElement* HTMLMediaElement::elementWithID(uint64_t id)
> {
>     if (id == HTMLMediaElementInvalidID)
>         return nullptr;
> 
>     return elementIDsToElements().get(id);
> }
> 
> static uint64_t nextElementID()
> {
>     static uint64_t elementID = 0;
>     return ++elementID;
> }
> #endif

In Keith's proposed scheme, the static functions need to go into the 
FILENAME namespace. The typedef probably should as well, since we'd 
want to ensure it doesn't clash with other hypothetical typedefs in 
other files. (The chances of another file having a typedef of the name 
IDToElementMap are probably rather low, but that's also true for the 
function names as well.) And *the uses of the static functions from 
outside the namespace will now need to be qualified as well* (thanks to 
Alicia for pointing that out; seems obvious, but I'd missed it). So the 
result, if we don't reorder anything, would have to look like this:

> #if ENABLE(MEDIA_SESSION)
> namespace FILENAME {

> typedef HashMap<uint64_t, HTMLMediaElement*> IDToElementMap;
> 
> static IDToElementMap& elementIDsToElements()
> {
>     static NeverDestroyed<IDToElementMap> map;
>     return map;
> }

> } // namespace FILENAME
> 
> HTMLMediaElement* HTMLMediaElement::elementWithID(uint64_t id)
> {
>     if (id == HTMLMediaElementInvalidID)
>         return nullptr;
> 
>     return FILENAME::elementIDsToElements().get(id);
> }

> namespace FILENAME {

> static uint64_t nextElementID()
> {
>     static uint64_t elementID = 0;
>     return ++elementID;
> }
> 
> } // namespace FILENAME
> 
> #endif

We could reorder functions and add more forward declarations to reduce 
the number of extra namespace declarations that we'll need. But there's 
nothing we can do to eliminate the mess caused by needing to use the 
extra FILENAME:: scope when using the static variables and functions 
outside the namespace:

    return FILENAME::elementIDsToElements().get(id);

If it's going to improve clean build times by 80%, then we should 
probably put up the this messiness to get the faster build... but 
surely nobody is going to be very fond of making our code messier. 
(Tangent: I expect this might discourage future use of file-static 
functions and encourage the use of private class member functions. 
Ryusuke and Darin have already shown why that's a bad idea.)

We would be better off *not* attempting to avoid naming clashes 
automatically, i.e. not using the FILENAME namespace. If we give up on 
automatically bundling sources together, and instead group them 
together manually or semi-manually in the build system so that adding a 
new file does not unexpectedly result in changing which other files get 
bundled together, then we can get the benefits of unified builds 
without needing to make significant modifications to our source code. 
We would only need to rename static variables and functions in the rare 
cases that there are actual name collisions with other files in the 
same bundle. I expect such cases will be quite rare. The cost is that 
we would have to manually manage which files get bundled together in 
CMake, rather than having this be determined automatically as under 
Keith's plan.

For example, currently in Source/WebCore/CMakeLists.txt we have:

> set(WebCore_SOURCES
>     Modules/airplay/WebKitPlaybackTargetAvailabilityEvent.cpp
> 
>     Modules/beacon/NavigatorBeacon.cpp
> 
>     Modules/cache/Cache.cpp
>     Modules/cache/CacheStorage.cpp
>     Modules/cache/CacheStorageConnection.cpp
>     Modules/cache/DOMCache.cpp
>     Modules/cache/DOMWindowCaches.cpp
>     Modules/cache/WorkerCacheStorageConnection.cpp
>     Modules/cache/WorkerGlobalScopeCaches.cpp
> 
>     # ...
> )

We could instead write something like this:

> ADD_SOURCES_BUNDLE(WebCore_SOURCES
>     Modules/airplay/WebKitPlaybackTargetAvailabilityEvent.cpp
>     Modules/beacon/NavigatorBeacon.cpp
> )
> 
> ADD_SOURCES_BUNDLE(WebCore_SOURCES
>     Modules/cache/Cache.cpp
>     Modules/cache/CacheStorage.cpp
>     Modules/cache/CacheStorageConnection.cpp
>     Modules/cache/DOMCache.cpp
>     Modules/cache/DOMWindowCaches.cpp
>     Modules/cache/WorkerCacheStorageConnection.cpp
>     Modules/cache/WorkerGlobalScopeCaches.cpp
> )

We'd have to figure out how to implement ADD_SOURCES_BUNDLE.

I expect manually determining which files to bundle together will be 
less optimal that automatically determining which files should be built 
together, since Keith has already figured out the ideal bundle size and 
our bundles are surely going to deviate from that, especially after 
files get added or removed. But making the bundles deterministic should 
eliminate the need for the FILENAME namespace, and we should still get 
a big speedup over where we are now, so that should be worth it. I'd 
much rather have additional complexity in our CMake build system than 
in our C++ sources. And it should be straightforward to remove this 
extra complexity from our build system when we decide to switch to C++ 
modules in the future.

Michael