RESOLVED FIXED Bug 94019
De-inline stuff from RenderStyle.h 
https://bugs.webkit.org/show_bug.cgi?id=94019
Summary De-inline stuff from RenderStyle.h
nbhargava
Reported 2012-08-14 13:50:35 PDT
De-inline stuff from RenderStyle.h
Attachments
Patch (15.22 KB, patch)
2012-08-14 13:52 PDT, nbhargava 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
nbhargava
Comment 1 2012-08-14 13:52:24 PDT
Created attachment 158409 [details] Patch
Eric Seidel (no email)
Comment 2 2012-08-14 14:10:56 PDT
Comment on attachment 158409 [details] Patch This is the perf sensitive part of the change. You'll need to watch the Chromium perf bots for any hiccup after this change. You can ask Tony G or James R how to find/read them. Most concerning is if the PLT results were to change. If we see a negative perf change from this, we'll need to report it here in this bug, and very likely roll this change out. Thanks!
WebKit Review Bot
Comment 3 2012-08-14 15:06:00 PDT
Comment on attachment 158409 [details] Patch Clearing flags on attachment: 158409 Committed r125612: <http://trac.webkit.org/changeset/125612>
WebKit Review Bot
Comment 4 2012-08-14 15:06:03 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 5 2012-08-15 16:49:23 PDT
Re-opened since this is blocked by 94159
Simon Fraser (smfr)
Comment 6 2012-08-15 16:51:27 PDT
Resolving again. However, I would have liked the commit message to say more about what performance analysis was done BEFORE the commit. I'd prefer not to just rely on perf bots for this.
Eric Seidel (no email)
Comment 7 2012-08-15 16:58:11 PDT
Could you please explain what perf analysis you would like done before commit? Particularly, what perf analysis you would perform yourself before making such a commit? I believe the perf bots provide much better numbers than individuals are able to produce on their machines. I think rollout here was arbitrary and pre-mature. The bots are actively being watched, as noted: https://bugs.webkit.org/show_bug.cgi?id=93629#c46
Eric Seidel (no email)
Comment 8 2012-08-15 17:00:36 PDT
I see, it was never rolled out. I remain interested in your suggestions. :)
Eric Seidel (no email)
Comment 10 2012-08-16 00:17:33 PDT
Thank you Tony.
John Mellor
Comment 11 2012-08-16 03:39:29 PDT
Comment on attachment 158409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158409&action=review > Source/WebCore/rendering/style/RenderStyle.h:-721 > - short verticalBorderSpacing() const { return inherited->vertical_border_spacing; } What do we gain from un-inlining these two getters? They don't seem to require Font, or any other expensive includes? > Source/WebCore/rendering/style/RenderStyle.h:-1139 > - void setLineHeight(Length v) { SET_VAR(inherited, line_height, v) } Ditto here, it doesn't seem feasible to remove the Length.h include. Is this change just for consistency (so all the line height methods are non-inline)? > Source/WebCore/rendering/style/RenderStyle.h:-1193 > - void setVerticalBorderSpacing(short v) { SET_VAR(inherited, vertical_border_spacing, v) } Ditto.
nbhargava
Comment 12 2012-08-16 09:53:51 PDT
(In reply to comment #11) > What do we gain from un-inlining these two getters? They don't seem to require Font, or any other expensive includes? inherited is of type StyleInheritedData, and StyleInheritedData.h includes Font. As a result, in order to effectively get rid of extraneous includes of Font from RenderStyle, you also have to remove function calls to things of StyleInheritedData. SET_VAR modifies the internal data of inherited, which is why those functions are de-inlined. In terms of rough numbers, on my machine I get a drop-off of ~260s of CPU time in a build (so a serialized compilation would be 260s faster) of Chromium when this change is made, along with the stuff from bug 93629. That being said, if there are worries about problems with run-time speed due to de-inlining, I'm very okay rolling this back.
Note You need to log in before you can comment on or make changes to this bug.