Bug 95107 – Split LayoutTypes.h to improve compile time

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

Split LayoutTypes.h to improve compile time

Created attachment 160755 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 160755 [details]
Patch

I'm not a huge fan of the new name.  LayoutTypes was already kinda the catch-all for these types.  I might have gone the other way and moved the functions out into a new header.  LayoutTypesInlines.h or similar?  Lets poll eae/smfr (sadly levi is on vacation).

[Eric Seidel (no email)]

Aside from the (to me) awkward naming, the change looks great!

Created attachment 160806 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 160806 [details]
Patch

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

Actually, I went to go find other Inlines.h examples for you, and foudn they're all called "*InlineMethods.h"  Seems we should match that pattern here.  (LayoutTypesInlineMethods.h would be the name.)

Again, this is ready for r+, but the misplaced comment seems confusing.  Since we're going one more round for the comment, if you could change the name one more time that would be ideal.  Sorry for the run-around. :(

> Source/WebCore/rendering/LayoutTypesInline.h:34
> +// These typedefs are being used to abstract layout and hit testing off
> +// of integers and eventually replace them with floats. Once this transition
> +// is complete, these types will be removed. Progress can be tracked at
> +// https://bugs.webkit.org/show_bug.cgi?id=60318

This seems misplaced?

[Eric Seidel (no email)]

Once again, I really apprecaite your work on compile times.  They make us all more productive in the end.

[Simon Fraser (smfr)]

Comment on attachment 160806 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        as many things. (Parts from LayoutTypes.h are expensive to include)

It would be nice to mention which parts.

Created attachment 160827 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 160827 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 160827 [details]
Patch

Clearing flags on attachment: 160827

Committed r126816: <http://trac.webkit.org/changeset/126816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Did you measure how much this improves compile times, or was this based on intuition?

A bit of both (though all of my hard measurements are based off of a Chromium build). LayoutTypes and all of its nested #includes take roughly 0.7s to compile. LayoutTypes is included in a lot of high-use header files, such as Node.h, but in reality the only thing that most header files needs from LayoutTypes.h is a declaration of LayoutUnit, LayoutRect, etc.

The inline methods are the only part of LayoutTypes.h that actually need full definitions of what the Layout elements amount to. Eliminating the inclusions of FractionalLayoutRect, FloatRect, etc. from files that didn't need them led to a decrease in aggregate CPU time by 4-5 minutes (so a fully serialized Chromium build would be 5 minutes faster).

I haven't explicitly measured the results on just a WebKit build, but I'd imagine that there would be some similar benefit.

[Alexey Proskuryakov]

Thank you!

[WebKit Review Bot]

Re-opened since this is blocked by 95310

[Emil A Eklund]

Thanks Nikhil! If you have any other ideas of how we can reduce the compile time penalty for LayoutTypes until we remove the abstraction please do tell.

[Emil A Eklund]

Marking as invalid as LayoutTypes.h is no more.