Bug 26103 – [GTK] Remove gtk/cairo/pango/freetype/fontconfig includes from the header files in WebCore

• Component: WebKitGTK
• Status: RESOLVED LATER

[Holger Freyther]

Use forward declarations to get rid of many many includes in a lot of different places. The idea is that this will speed up compilation time.

[Holger Freyther]

Created attachment 30814 [details]
The patch...

 cat 0008-2009-05-31-Holger-Hans-Peter-Freyther-zecke-selfi.patch | diffstat
 ChangeLog                                           |   33 ++++++++++++++++++++
 platform/PopupMenu.h                                |    3 +
 platform/graphics/GlyphBuffer.h                     |   31 +++++++-----------
 platform/graphics/Pattern.h                         |    2 -
 platform/graphics/SimpleFontData.h                  |    2 -
 platform/graphics/cairo/FontCairo.cpp               |    6 +++
 platform/graphics/cairo/ImageBufferData.h           |    2 -
 platform/graphics/gtk/FontCacheGtk.cpp              |    4 ++
 platform/graphics/gtk/FontCustomPlatformData.cpp    |    3 +
 platform/graphics/gtk/FontPlatformData.h            |   15 ++++++---
 platform/graphics/gtk/GlyphPageTreeNodeGtk.cpp      |    3 +
 platform/graphics/gtk/GlyphPageTreeNodePango.cpp    |    1 
 platform/graphics/gtk/MediaPlayerPrivateGStreamer.h |    6 ++-
 platform/graphics/gtk/SimpleFontDataGtk.cpp         |    5 ++-
 platform/graphics/gtk/SimpleFontDataPango.cpp       |    4 +-
 platform/graphics/transforms/TransformationMatrix.h |    2 -
 platform/gtk/PasteboardHelper.h                     |    3 +
 platform/gtk/RenderThemeGtk.h                       |    3 +
 platform/network/ResourceHandleInternal.h           |    9 +++--
 platform/network/soup/ResourceRequest.h             |    4 +-
 20 files changed, 102 insertions(+), 39 deletions(-)

[Adrian Bunk]

I wonder whether this approach can result in any noticable compile time improvement at all (e.g. many source files in applications that include webkit headers anyway already include gtk.h).

Do you have any proof that this actually speeds up the build time of applications?

[Holger Freyther]

(In reply to comment #2)
> I wonder whether this approach can result in any noticable compile time
> improvement at all (e.g. many source files in applications that include webkit
> headers anyway already include gtk.h).
> 
> Do you have any proof that this actually speeds up the build time of
> applications?

I don't have any proof but the goal is not to speed up build time of applications using WebKit/GTK+. The goal is to speed up compiling of WebKit itself. I have changed the summary to explicitly mention WebCore. As of today WebCore/css/MediaQueryEvaluator.cpp is including libsoup/soup.h. This will result in including glib-object, glib. I had a hard time finding any file in WebCore that doesn't include any of gtk/cairo/pango/freetype/fontconfig.

Let me try to produce some numbers...

[Holger Freyther]

Numbers from my amd64 freebsd machine with time gmake > /dev/null:

Without patch

real    41m23.492s
user    30m36.283s
sys     10m24.233s


With patch:
real    39m27.646s
user    29m55.705s
sys     9m5.392s

[Adrian Bunk]

Disclaimer: I am not a WebKit developer.

First of all, sorry for misunderstanding the purpose of your patch.

But I do dislike your patch since it breaks the proper abstractions.

An example:

-        gsize m_bufferSize, m_total;
+        unsigned long long m_bufferSize, m_total;

gsize might be "unsigned long long" on your FreeBSD machine.

But not on my x86 Linux machine (with a 64bit userspace it's "unsigned long", and with a 32bit userspace it's "unsigned int").

glib has an autoconf test for determining the type of gsize. Do you really want to duplicate that in WebKit?

[Holger Freyther]

(In reply to comment #5)
> Disclaimer: I am not a WebKit developer.
> 
> First of all, sorry for misunderstanding the purpose of your patch.
> 
> But I do dislike your patch since it breaks the proper abstractions.
> An example:

Okay, I think you are slightly off again + my description not being too good. My goal is to not have any glib/glib.h, cairo.h in WebCore header files as this is increasing compile times (see this simple patch can save quite some time).

Way to get there include:
  - use forward declarations... and they are typesafe, e.g. if I declare typedef struct _GtkEntry GtkWidget the compiler will shout at me.

  - do not use Glib types when not necessary. Using unsigned long long was a bit too much to store the value 8192, so an updated patch will use unsigned. See how I just didn't do typedef unsigned long gsize?

  - Create "convertable" types and this is where I expected discussion. :)


Why abstraction is not broken. The ResourceHandleClient::didReceiveData and other parts using size in WebCore do not use gsize. So we already have a place when going from gsize to int (and so far it is safe as we only read about to 8k), moving the frontier does not break the abstraction...


> But not on my x86 Linux machine (with a 64bit userspace it's "unsigned long",
> and with a 32bit userspace it's "unsigned int").

I assume you mean GNU/Linux and GNU userspace (gcc, glibc)?

[Adrian Bunk]

(In reply to comment #6)
> (In reply to comment #5)
> > Disclaimer: I am not a WebKit developer.
> > 
> > First of all, sorry for misunderstanding the purpose of your patch.
> > 
> > But I do dislike your patch since it breaks the proper abstractions.
> > An example:
> 
> Okay, I think you are slightly off again + my description not being too good.
> My goal is to not have any glib/glib.h, cairo.h in WebCore header files as this
> is increasing compile times (see this simple patch can save quite some time).
> 
> Way to get there include:
>   - use forward declarations... and they are typesafe, e.g. if I declare
> typedef struct _GtkEntry GtkWidget the compiler will shout at me.
> 
>   - do not use Glib types when not necessary. Using unsigned long long was a
> bit too much to store the value 8192, so an updated patch will use unsigned.
> See how I just didn't do typedef unsigned long gsize?
>...

OK, so this seems to work.

But what would e.g. happen if with a future (ABI-incompatible) version of glib "typedef gint gboolean;" would no longer be true?

Overally, my personal opinion is that your patch is going into the wrong direction - even if my suspicion that it might also be wrong on some technical details turns out to be wrong.

But I am not a WebKit developer, I only ran into this bug when looking for completely different issues, and I'm gonna keep my mouth shut on it now.

[Holger Freyther]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Disclaimer: I am not a WebKit developer.
> > > 
> > > First of all, sorry for misunderstanding the purpose of your patch.
> > > 
> > > But I do dislike your patch since it breaks the proper abstractions.
> > > An example:
> > 
> > Okay, I think you are slightly off again + my description not being too good.
> > My goal is to not have any glib/glib.h, cairo.h in WebCore header files as this
> > is increasing compile times (see this simple patch can save quite some time).
> > 
> > Way to get there include:
> >   - use forward declarations... and they are typesafe, e.g. if I declare
> > typedef struct _GtkEntry GtkWidget the compiler will shout at me.
> > 
> >   - do not use Glib types when not necessary. Using unsigned long long was a
> > bit too much to store the value 8192, so an updated patch will use unsigned.
> > See how I just didn't do typedef unsigned long gsize?
> >...
> 
> OK, so this seems to work.
> 
> But what would e.g. happen if with a future (ABI-incompatible) version of glib
> "typedef gint gboolean;" would no longer be true?

then your compiler will give you an error about incompatible types.

> Overally, my personal opinion is that your patch is going into the wrong
> direction - even if my suspicion that it might also be wrong on some technical
> details turns out to be wrong.

Please elaborate on it. Do you disagree with forward declarations in general? for glib types? I know it is a bigger difference from C to C++. But forward declarations are a pretty common way to speed compilation up. We use it extensively in WebKit. Or in a more dramatic way... why do you think MediaQueryEvaluator.cpp needs to know about GIO?

[Adrian Bunk]

(In reply to comment #8)
>...
> > Overally, my personal opinion is that your patch is going into the wrong
> > direction - even if my suspicion that it might also be wrong on some technical
> > details turns out to be wrong.
> 
> Please elaborate on it. Do you disagree with forward declarations in general?
> for glib types? I know it is a bigger difference from C to C++. But forward
> declarations are a pretty common way to speed compilation up. We use it
> extensively in WebKit. Or in a more dramatic way... why do you think
> MediaQueryEvaluator.cpp needs to know about GIO?

The glib types and the changes leading to the COMPILE_ASSERT() are what I dislike about your patch.

But I get your point, and especially due to the fact that something like #include <glib/gtypes.h> (which might be the best way to get the glib types without pulling in all glib headers) is strongly deprecated your patch might be the way to go for speeding up the compilation.

[Eric Seidel (no email)]

Comment on attachment 30814 [details]
The patch...

This looks fine to me.  Why though?  I assume for compile performance?

This comment should reference the COMPILE_ASSERT:
+// This is in fact a cairo_glyph_t make sure this keeps matching

I don't really believe the numbers you posted.  The change was small (seconds).  Did you run the numbers multiple times and confirm that your numbers are consistent?

I'm really just gonna trust you on this one.

[Eric Seidel (no email)]

Did you decide to go forward with this Holger?

[Holger Freyther]

(In reply to comment #11)
> Did you decide to go forward with this Holger?

The cairo change itself is a bit controversial. There will be a big KDE/GNOME summit starting the 3rd of July and I will attempt to discuss this change with Gustavo and Xan in person to get their opinion on it. If you want this change to drop out of the commit queue then do so. 

[Eric Seidel (no email)]

Comment on attachment 30814 [details]
The patch...

Removing review flag.  Feel free to re-flag for review once there is consensus from the gtk folks.

[Eric Seidel (no email)]

Comment on attachment 30814 [details]
The patch...

Removing review flag.  Feel free to re-flag for review once there is consensus from the gtk folks.

[Gustavo Noronha (kov)]

Since I promised to make up my mind on this, here I go =D. With the numbers we currently have, I don't think the performance gains outweight the complexity this may bring in a significant way to make it desirable, so -1 from me, for now, unless you would like to get the uncontroversial ones in, such as, say:

typedef struct _cairo cairo_t;

We already use this kind of typedef all around, so I see no problem in having some more. The one I am worried about is the full struct definition.

[Holger Freyther]

(In reply to comment #15)

> We already use this kind of typedef all around, so I see no problem in having
> some more. The one I am worried about is the full struct definition.

The problem is that the implementation is in the header file and we need the full blown cairo.h. The result is that in many many places we have all cairo includes.
I will send an updated patch without the struct reinterpretation...

[Martin Robinson]

We're doing this gradually now. This patch has certainly bitrotted. I'll be happy to review any new patches against trunk fixing these issues.