| Summary: | De-inline stuff from RenderStyle.h | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | nbhargava | ||||
| Component: | New Bugs | Assignee: | nbhargava | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | darin, eric, jamesr, rniwa, simon.fraser, slewis, tony, tonyg, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Bug Depends on: | 94159 | ||||||
| Bug Blocks: | 93629 | ||||||
| Attachments: |
|
||||||
|
Description
nbhargava
2012-08-14 13:50:35 PDT
Created attachment 158409 [details]
Patch
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!
Comment on attachment 158409 [details] Patch Clearing flags on attachment: 158409 Committed r125612: <http://trac.webkit.org/changeset/125612> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 94159 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. 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 I see, it was never rolled out. I remain interested in your suggestions. :) 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 Thank you Tony. 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. |