Bug 93629 – Reduce Font.h includes across project -- improves RenderObject.h compile time

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Reduce Font.h includes across project -- improves RenderObject.h compile time

Created attachment 157481 [details]
Patch

Created attachment 157493 [details]
Patch

[Build Bot]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13464377

[Build Bot]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13457907

[Early Warning System Bot]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13464393

[Eric Seidel (no email)]

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.

[Early Warning System Bot]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13464397

[WebKit Review Bot]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13460630

[Gyuyoung Kim]

Comment on attachment 157493 [details]
Patch

Attachment 157493 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13456985

Created attachment 157757 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 157757 [details]
Patch

Attachment 157757 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13473394

Created attachment 157760 [details]
Patch

Created attachment 157762 [details]
Patch for landing

Created attachment 157764 [details]
Patch for landing

Created attachment 157765 [details]
Patch for landing

Created attachment 157767 [details]
Patch for landing

[Alexey Proskuryakov]

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.

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.

Created attachment 157778 [details]
Patch

[WebKit Review Bot]

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.

Created attachment 157783 [details]
Patch

[Build Bot]

Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13480016

[Gyuyoung Kim]

Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13461979

[Build Bot]

Comment on attachment 157783 [details]
Patch

Attachment 157783 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13464889

Created attachment 157845 [details]
Patch

[Build Bot]

Comment on attachment 157845 [details]
Patch

Attachment 157845 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13480101

Created attachment 158029 [details]
Patch

Created attachment 158056 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 158056 [details]
Patch

Set cq+ to let perfalizer grab the pathc.

[Ryosuke Niwa]

Comment on attachment 158056 [details]
Patch

Removing cq+.

[Perf EWS]

Created attachment 158183 [details]
Performance tests results for 158056

[Eric Seidel (no email)]

Ryosuke: can you explain what those perf numbers mean to us lay-folk?

[Ryosuke Niwa]

(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 :(

[Eric Seidel (no email)]

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.

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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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.

[Darin Adler]

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.

> 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.

[Eric Seidel (no email)]

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.

Created attachment 158453 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Early Warning System Bot]

Comment on attachment 158453 [details]
Patch

Attachment 158453 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13509004

[Darin Adler]

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?

Created attachment 158657 [details]
Patch

(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.

[Eric Seidel (no email)]

Comment on attachment 158657 [details]
Patch

The inlining change seems to have stuck.  Lets land this part.

[WebKit Review Bot]

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

Created attachment 160003 [details]
Patch for landing

Created attachment 160004 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160004 [details]
Patch

Clearing flags on attachment: 160004

Committed r126359: <http://trac.webkit.org/changeset/126359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.