Bug 94019 – De-inline stuff from RenderStyle.h

• Component: New Bugs
• Status: RESOLVED FIXED

De-inline stuff from RenderStyle.h

Created attachment 158409 [details]
Patch

[Eric Seidel (no email)]

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 on attachment 158409 [details]
Patch

Clearing flags on attachment: 158409

Committed r125612: <http://trac.webkit.org/changeset/125612>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 94159

[Simon Fraser (smfr)]

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)]

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)]

I see, it was never rolled out.  I remain interested in your suggestions. :)

[Tony Chang]

FWIW, the Chromium page cyclers didn't seem to regress:
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/moz/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl1/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/intl2/report.html?history=150&rev=-1

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/moz/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl2/report.html?history=150&rev=-1

http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/intl1/report.html?history=150&rev=-1
http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/intl2/report.html?history=150&rev=-1

[Eric Seidel (no email)]

Thank you Tony.

[John Mellor]

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.

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