Reduce Font.h includes across project -- improves RenderObject.h compile time
Created attachment 157481 [details] Patch
Created attachment 157493 [details] Patch
Comment on attachment 157493 [details] Patch Attachment 157493 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13464377
Comment on attachment 157493 [details] Patch Attachment 157493 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13457907
Comment on attachment 157493 [details] Patch Attachment 157493 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13464393
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 on attachment 157493 [details] Patch Attachment 157493 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13464397
Comment on attachment 157493 [details] Patch Attachment 157493 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13460630
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
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
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
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
Comment on attachment 157783 [details] Patch Attachment 157783 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13480016
Comment on attachment 157783 [details] Patch Attachment 157783 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13461979
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
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
Comment on attachment 158056 [details] Patch Set cq+ to let perfalizer grab the pathc.
Comment on attachment 158056 [details] Patch Removing cq+.
Created attachment 158183 [details] Performance tests results for 158056
Ryosuke: can you explain what those perf numbers mean to us lay-folk?
(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 :(
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.
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?
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 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.
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
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 on attachment 158453 [details] Patch Attachment 158453 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13509004
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.
Comment on attachment 158657 [details] Patch The inlining change seems to have stuck. Lets land this part.
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
Comment on attachment 160004 [details] Patch Clearing flags on attachment: 160004 Committed r126359: <http://trac.webkit.org/changeset/126359>
All reviewed patches have been landed. Closing bug.