Split LayoutTypes.h to improve compile time
Created attachment 160755 [details] Patch
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).
Aside from the (to me) awkward naming, the change looks great!
Created attachment 160806 [details] Patch
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?
Once again, I really apprecaite your work on compile times. They make us all more productive in the end.
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
Comment on attachment 160827 [details] Patch LGTM.
Comment on attachment 160827 [details] Patch Clearing flags on attachment: 160827 Committed r126816: <http://trac.webkit.org/changeset/126816>
All reviewed patches have been landed. Closing bug.
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.
Thank you!
Re-opened since this is blocked by 95310
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.
Marking as invalid as LayoutTypes.h is no more.