Bug 178968

Summary: [GTK] Build more files under WebCore as unified sources
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bugs-noreply, keith_miller, mcatanzaro
Priority: P2    
Version: Other   
Hardware: PC   
OS: Linux   
Bug Depends on: 178964    
Bug Blocks:    
Attachments:
Description Flags
Patch keith_miller: review+

Description Michael Catanzaro 2017-10-27 17:58:16 PDT
Build more files under WebCore as unified sources (after landing the patch in bug #178964).
Comment 1 Michael Catanzaro 2017-10-30 20:51:53 PDT
It will take some work (and understanding of Ruby) to be able to build WebCorePlatformGTK and WebCorePlatformGTK2 using unified sources, since they are two different build targets that are built with the same sources, and there is a useful check to ensure the same file is not accidentally listed twice that I don't want to remove. Plus I don't really understand that code.

But there are still many files here that can easily be built unified.
Comment 2 Michael Catanzaro 2017-10-30 20:53:18 PDT
Created attachment 325422 [details]
Patch
Comment 3 Keith Miller 2017-10-30 21:18:40 PDT
Comment on attachment 325422 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325422&action=review

> Source/WebCore/SourcesWPE.txt:40
> +# This can't go into SourcesGLib.txt because it has to go into WebCorePlatformGTK.

# => //
Comment 4 Keith Miller 2017-10-30 22:28:07 PDT
Comment on attachment 325422 [details]
Patch

r=me if you fix the WPE issue.
Comment 5 Michael Catanzaro 2017-10-31 08:08:10 PDT
(In reply to Keith Miller from comment #4)
> Comment on attachment 325422 [details]
> Patch
> 
> r=me if you fix the WPE issue.

That's not really possible to fix, which is why I removed the FIXME. The problem is that for GTK it needs to be complied into a different build target than all the other files in SourcesGLib. It will just have to be special, I guess.
Comment 6 Michael Catanzaro 2017-10-31 08:10:51 PDT
Oh wait, I misunderstood... I see now WPE is broken because I used # instead of //. Comment #3. Yeah.
Comment 7 Michael Catanzaro 2017-10-31 08:16:11 PDT
Committed r224233: <https://trac.webkit.org/changeset/224233>
Comment 8 Konstantin Tokarev 2018-02-07 03:31:21 PST
Is it really worthwhile? I don't have any statistics, but from my personal experience it's rather likely that GTK developers should modify platform-specific files more often than port-independent core during their work, and having files built as a part of unified system adds penalty to incremental rebuild time. OTOH, platform-specific files take a small fraction of total build time so speed up in complete rebuild is not so significant.
Comment 9 Michael Catanzaro 2018-02-07 05:50:28 PST
(In reply to Konstantin Tokarev from comment #8)
> Is it really worthwhile? I don't have any statistics, but from my personal
> experience it's rather likely that GTK developers should modify
> platform-specific files more often than port-independent core during their
> work, and having files built as a part of unified system adds penalty to
> incremental rebuild time. OTOH, platform-specific files take a small
> fraction of total build time so speed up in complete rebuild is not so
> significant.

The platform-specific files actually do have a significant impact on build time. The WPE port builds roughly 25% faster than the GTK port, which is insane. I'm hoping to cut down on this by unifying as much as possible, notably the GTK DOM API, which seems to be one of the major differences between GTK and WPE.

Current limitations:

 * The sources under WebCore/platform are difficult to unify, because the build files rely on USE_[LIBRARY] style flags instead of feature flags defined in WebKitFeatures.cmake. I'm not certain, but I don't believe we can use those in conditionals inside the Sources.txt.
 * We can only unify files that are built into a WebKit framework declared with the WEBKIT_FRAMEWORK macros. This means it's hard to unify the PluginProcessGTK2 build, which is the other main difference between GTK and WPE.
Comment 10 Konstantin Tokarev 2018-02-07 06:01:28 PST
(In reply to Michael Catanzaro from comment #9)

Thanks for explanation.

> The platform-specific files actually do have a significant impact on build
> time. The WPE port builds roughly 25% faster than the GTK port, which is
> insane. I'm hoping to cut down on this by unifying as much as possible,
> notably the GTK DOM API, which seems to be one of the major differences
> between GTK and WPE.

If only DOM API is unified, could it make a qualitative change?

> 
> Current limitations:
> 
>  * The sources under WebCore/platform are difficult to unify, because the
> build files rely on USE_[LIBRARY] style flags instead of feature flags
> defined in WebKitFeatures.cmake. I'm not certain, but I don't believe we can
> use those in conditionals inside the Sources.txt.
>  * We can only unify files that are built into a WebKit framework declared
> with the WEBKIT_FRAMEWORK macros. This means it's hard to unify the
> PluginProcessGTK2 build, which is the other main difference between GTK and
> WPE.

It may be useful to factorize WEBKIT_FRAMEWORK to be useful for non-libraries, and/or use modern property-based approach instead of relying on variable names. (Actually, I think having unified variable names in all targets is a good thing)
Comment 11 Michael Catanzaro 2018-02-07 08:31:38 PST
(In reply to Konstantin Tokarev from comment #10)
> If only DOM API is unified, could it make a qualitative change?

I haven't tried it yet. I'll try after bug #182450 lands.