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
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
