Bug 93629 - Reduce Font.h includes across project -- improves RenderObject.h compile time
Summary: Reduce Font.h includes across project -- improves RenderObject.h compile time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: nbhargava
URL:
Keywords:
Depends on: 94019
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-09 10:24 PDT by nbhargava
Modified: 2012-08-22 15:25 PDT (History)
16 users (show)

See Also:


Attachments
Patch (40.91 KB, patch)
2012-08-09 10:28 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (40.77 KB, patch)
2012-08-09 11:06 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (44.00 KB, patch)
2012-08-10 10:43 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (44.48 KB, patch)
2012-08-10 11:09 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch for landing (44.49 KB, patch)
2012-08-10 11:14 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch for landing (45.51 KB, patch)
2012-08-10 11:18 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch for landing (46.06 KB, patch)
2012-08-10 11:21 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch for landing (46.50 KB, patch)
2012-08-10 11:27 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (44.93 KB, patch)
2012-08-10 11:44 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (44.93 KB, patch)
2012-08-10 11:53 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (49.59 KB, patch)
2012-08-10 17:51 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (50.62 KB, patch)
2012-08-13 09:58 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (51.17 KB, patch)
2012-08-13 11:36 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Performance tests results for 158056 (92.33 KB, text/html)
2012-08-13 18:49 PDT, Perf EWS
no flags Details
JS Benchmark Results (5.01 KB, text/plain)
2012-08-14 10:10 PDT, nbhargava
no flags Details
Patch (37.00 KB, patch)
2012-08-14 17:17 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (37.50 KB, patch)
2012-08-15 16:25 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch for landing (31.97 KB, patch)
2012-08-22 13:57 PDT, nbhargava
no flags Details | Formatted Diff | Diff
Patch (37.54 KB, patch)
2012-08-22 14:00 PDT, nbhargava
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description nbhargava 2012-08-09 10:24:47 PDT
Reduce Font.h includes across project -- improves RenderObject.h compile time
Comment 1 nbhargava 2012-08-09 10:28:02 PDT
Created attachment 157481 [details]
Patch
Comment 2 nbhargava 2012-08-09 11:06:00 PDT
Created attachment 157493 [details]
Patch
Comment 3 Build Bot 2012-08-09 11:31:22 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13464377
Comment 4 Build Bot 2012-08-09 11:43:06 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13457907
Comment 5 Early Warning System Bot 2012-08-09 12:17:40 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13464393
Comment 6 Eric Seidel (no email) 2012-08-09 12:22:49 PDT
Comment on attachment 157493 [details]
Patch

I'm not sure you can take all of those RenderStyle functions out of lines w/o degrading performance.
Comment 7 Early Warning System Bot 2012-08-09 12:48:51 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13464397
Comment 8 WebKit Review Bot 2012-08-09 12:49:34 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13460630
Comment 9 Gyuyoung Kim 2012-08-09 13:03:48 PDT
Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13456985
Comment 10 nbhargava 2012-08-10 10:43:14 PDT
Created attachment 157757 [details]
Patch
Comment 11 Gyuyoung Kim 2012-08-10 10:56:17 PDT
Comment on attachment 157757 [details]
Patch

Attachment 157757 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13473394
Comment 12 nbhargava 2012-08-10 11:09:11 PDT
Created attachment 157760 [details]
Patch
Comment 13 nbhargava 2012-08-10 11:14:24 PDT
Created attachment 157762 [details]
Patch for landing
Comment 14 nbhargava 2012-08-10 11:18:10 PDT
Created attachment 157764 [details]
Patch for landing
Comment 15 nbhargava 2012-08-10 11:21:28 PDT
Created attachment 157765 [details]
Patch for landing
Comment 16 nbhargava 2012-08-10 11:27:43 PDT
Created attachment 157767 [details]
Patch for landing
Comment 17 Alexey Proskuryakov 2012-08-10 11:32:09 PDT
Comment on attachment 157767 [details]
Patch for landing

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

I agree with Eric that de-inlining functions looks suspicious.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

This patch has never been r+ed as far as I can tell - why do you not set the "review?" flag any more?

> Source/WebCore/ChangeLog:8
> +        RenderStyle.cpp and RenderStyle.h are modified to remove dependencies onFont.h. The remaining files are cleaned up if they do need Font.h. The biggest impact is that RenderObject.h no longer always includes Font.h

This line is long, please wrap it to the regular width as seen in other entries.
Comment 18 nbhargava 2012-08-10 11:41:09 PDT
I plan to run performance tests soon in order to check whether the de-inlining causes a regression in performance. I'm having a little trouble though and am waiting for rniwa for some guidance on those. In the mean time I just want to make sure that it passes EWS since I'd have to do that anyways if there are no performance impacts.

My bad on the other two things. Will fix those.

(In reply to comment #17)
> (From update of attachment 157767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157767&action=review
> 
> I agree with Eric that de-inlining functions looks suspicious.
> 
> > Source/WebCore/ChangeLog:6
> > +        Reviewed by NOBODY (OOPS!).
> 
> This patch has never been r+ed as far as I can tell - why do you not set the "review?" flag any more?
> 
> > Source/WebCore/ChangeLog:8
> > +        RenderStyle.cpp and RenderStyle.h are modified to remove dependencies onFont.h. The remaining files are cleaned up if they do need Font.h. The biggest impact is that RenderObject.h no longer always includes Font.h
> 
> This line is long, please wrap it to the regular width as seen in other entries.
Comment 19 nbhargava 2012-08-10 11:44:37 PDT
Created attachment 157778 [details]
Patch
Comment 20 WebKit Review Bot 2012-08-10 11:48:49 PDT
Attachment 157778 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 59 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 nbhargava 2012-08-10 11:53:51 PDT
Created attachment 157783 [details]
Patch
Comment 22 Build Bot 2012-08-10 12:29:08 PDT
Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13480016
Comment 23 Gyuyoung Kim 2012-08-10 13:07:38 PDT
Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13461979
Comment 24 Build Bot 2012-08-10 16:44:24 PDT
Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13464889
Comment 25 nbhargava 2012-08-10 17:51:12 PDT
Created attachment 157845 [details]
Patch
Comment 26 Build Bot 2012-08-10 18:32:03 PDT
Comment on attachment 157845 [details]
Patch

Attachment 157845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13480101
Comment 27 nbhargava 2012-08-13 09:58:03 PDT
Created attachment 158029 [details]
Patch
Comment 28 nbhargava 2012-08-13 11:36:26 PDT
Created attachment 158056 [details]
Patch
Comment 29 Ryosuke Niwa 2012-08-13 14:37:13 PDT
Comment on attachment 158056 [details]
Patch

Set cq+ to let perfalizer grab the pathc.
Comment 30 Ryosuke Niwa 2012-08-13 14:38:00 PDT
Comment on attachment 158056 [details]
Patch

Removing cq+.
Comment 31 Perf EWS 2012-08-13 18:49:50 PDT
Created attachment 158183 [details]
Performance tests results for 158056
Comment 32 Eric Seidel (no email) 2012-08-13 18:53:06 PDT
Ryosuke: can you explain what those perf numbers mean to us lay-folk?
Comment 33 Ryosuke Niwa 2012-08-13 18:58:42 PDT
(In reply to comment #32)
> Ryosuke: can you explain what those perf numbers mean to us lay-folk?

The first bar is without the patch and the second bar is with the patch. Sorry, I thought I added labels but apparently I have not :(
Comment 34 Eric Seidel (no email) 2012-08-14 01:05:46 PDT
More specifically: are bigger bars good? bad?  Could this just be noise?  a bunch of bars got bigger some got smaller?  It's unclear what this means.
Comment 35 nbhargava 2012-08-14 10:10:46 PDT
Created attachment 158357 [details]
JS Benchmark Results

I added the results from some JS benchmark suites doing comparisons before and after the patch was applied. I'm not sure if these tests were already covered by Perfalizer but I figured that they're worth including.

There are a few minor differences between the versions, but I'm not sure whether they're significant or the result of noise.
Comment 36 Eric Seidel (no email) 2012-08-14 12:28:00 PDT
Comment on attachment 158056 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:68
> +const FontMetrics& RenderStyle::fontMetrics() const { return inherited->font.fontMetrics(); }
> +const Font& RenderStyle::font() const { return inherited->font; }

Huh?  Does WebProcess not include RenderSTyle.cpp?
Comment 37 Eric Seidel (no email) 2012-08-14 12:30:35 PDT
What I learned from talking to rniwa, is that the results of run-performance-tests are largely useless at this point.  The tool is simply too young to help us make these decisions.  I expect we're going to end up just landing this and watching the Chromium perf bots.  You'll just have to be aware that we may roll this out if the bots believe this to be a regression on the PLT or otherwise.
Comment 38 Darin Adler 2012-08-14 12:31:30 PDT
Comment on attachment 158056 [details]
Patch

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

So what have we learned about the performance impact of removing all this inlining?

I suggest we make a first patch that just removes the inlining. Then we can follow up with the patch that makes all the include changes to get the build time improvements.

> Source/WebCore/rendering/style/RenderStyle.h:1107
> +    bool setFontDescription(const FontDescription& v);

Argument names should be omitted in cases like this.

> Source/WebCore/rendering/style/RenderStyle.h:1112
> +    void setColor(const Color& v);

And this.

> Source/WebCore/rendering/style/RenderStyle.h:1140
> +    void setWordSpacing(int v);
> +    void setLetterSpacing(int v);

And these.

> Source/WebCore/rendering/style/RenderStyle.h:1185
> +    void setListStyleImage(PassRefPtr<StyleImage> v);

Here too.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:68
>> +const FontMetrics& RenderStyle::fontMetrics() const { return inherited->font.fontMetrics(); }
>> +const Font& RenderStyle::font() const { return inherited->font; }
> 
> Huh?  Does WebProcess not include RenderSTyle.cpp?

Yes, as Eric is implying, this code change is wrong. The correct fix is to add exports to WebCore.exp.in rather than copying WebCore code into WebKit2.
Comment 39 nbhargava 2012-08-14 12:42:58 PDT
> I suggest we make a first patch that just removes the inlining. Then we can follow up with the patch that makes all the include changes to get the build time improvements.

Makes sense to me. Should I split this into two bug reports or just leave it here and make the patches more incremental?

> So what have we learned about the performance impact of removing all this inlining?

In terms of performance, the outputted results make it seem like there isn't really an impact. That being said, I'm not familiar with the expected outputs at all, so I'm hoping someone else can chime in here as to whether these are effective.


Thanks for the additional comments. I will make those changes promptly.
Comment 40 Eric Seidel (no email) 2012-08-14 12:56:50 PDT
Each patch should have it's own bug report.  So I'd just spin off a new bug for one of those patches and mark it as blocking this bug.
Comment 41 nbhargava 2012-08-14 17:17:02 PDT
Created attachment 158453 [details]
Patch
Comment 42 Eric Seidel (no email) 2012-08-14 17:33:15 PDT
Comment on attachment 158453 [details]
Patch

I would argue we should wait to land this until we believe the perf from the previous change to be OK.  Otherwise we're just signing ourselves up for 2 rollouts instead of one. :)
Comment 43 Early Warning System Bot 2012-08-14 18:52:16 PDT
Comment on attachment 158453 [details]
Patch

Attachment 158453 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13509004
Comment 44 Darin Adler 2012-08-15 16:03:24 PDT
Comment on attachment 158453 [details]
Patch

Qt build failure looks like a missing include of StyleInheritedData.h.

How did the un-inlining change go? What performance data do we have?
Comment 45 nbhargava 2012-08-15 16:25:59 PDT
Created attachment 158657 [details]
Patch
Comment 46 nbhargava 2012-08-15 16:29:47 PDT
(In reply to comment #44)
> How did the un-inlining change go? What performance data do we have?

The other patch landed mid-day yesterday and since then I've been looking at webkit-perf.appspot.com periodically, paying special notice to the PageLoad tests to see if this brought about any changes. However, as far as I can tell, there haven't been any significant changes.

I'm not sure though how long it takes for a performance regression to be noticed, so I'll defer to the authority of someone else as to when or if this change should proceed.
Comment 47 Eric Seidel (no email) 2012-08-21 16:18:33 PDT
Comment on attachment 158657 [details]
Patch

The inlining change seems to have stuck.  Lets land this part.
Comment 48 WebKit Review Bot 2012-08-21 16:36:06 PDT
Comment on attachment 158657 [details]
Patch

Rejecting attachment 158657 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t struct WebCore::FontDescription'
Source/WebCore/rendering/style/RenderStyle.h:107: error: forward declaration of 'const struct WebCore::FontDescription'
make: *** [out/Release/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources01.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** [out/Release/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources05.o] Error 1
make: *** [out/Release/obj.target/webcore_bindings/gen/webkit/bindings/V8DerivedSources02.o] Error 1

Full output: http://queues.webkit.org/results/13564040
Comment 49 nbhargava 2012-08-22 13:57:44 PDT
Created attachment 160003 [details]
Patch for landing
Comment 50 nbhargava 2012-08-22 14:00:29 PDT
Created attachment 160004 [details]
Patch
Comment 51 WebKit Review Bot 2012-08-22 15:25:38 PDT
Comment on attachment 160004 [details]
Patch

Clearing flags on attachment: 160004

Committed r126359: <http://trac.webkit.org/changeset/126359>
Comment 52 WebKit Review Bot 2012-08-22 15:25:46 PDT
All reviewed patches have been landed.  Closing bug.