Bug 109909 - [Refactoring] Make m_state an on-stack object
Summary: [Refactoring] Make m_state an on-stack object
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on: 110719
Blocks: 89879
  Show dependency treegraph
 
Reported: 2013-02-15 00:50 PST by Takashi Sakamoto
Modified: 2013-03-04 22:19 PST (History)
13 users (show)

See Also:


Attachments
Patch (233.26 KB, patch)
2013-02-15 01:24 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (233.26 KB, patch)
2013-02-20 00:04 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch for landing (233.24 KB, patch)
2013-02-20 19:15 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch(revised) (234.34 KB, patch)
2013-02-28 10:37 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (234.27 KB, patch)
2013-03-04 21:22 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2013-02-15 00:50:50 PST
Currently m_state is a member variable of StyleResolver. Make the m_state an on-stack object.
Comment 1 Takashi Sakamoto 2013-02-15 01:24:51 PST
Created attachment 188509 [details]
Patch
Comment 2 Takashi Sakamoto 2013-02-20 00:04:57 PST
Created attachment 189257 [details]
Patch
Comment 3 Antti Koivisto 2013-02-20 05:39:04 PST
Comment on attachment 189257 [details]
Patch

r=me
Comment 4 Takashi Sakamoto 2013-02-20 19:15:25 PST
Created attachment 189449 [details]
Patch for landing
Comment 5 WebKit Review Bot 2013-02-20 19:41:27 PST
Comment on attachment 189449 [details]
Patch for landing

Clearing flags on attachment: 189449

Committed r143556: <http://trac.webkit.org/changeset/143556>
Comment 6 WebKit Review Bot 2013-02-20 19:41:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Hajime Morrita 2013-02-24 20:04:45 PST
(In reply to comment #7)
> This looks like 2-3% performance regression in Parser/html5-full-render:
Thanks for the catch! Takashi is out until next week. Let me roll this out for now.
Comment 9 Antti Koivisto 2013-02-25 03:30:59 PST
Rollout did indeed fix the regression.
Comment 10 Hajime Morrita 2013-02-26 09:42:12 PST
Reopening (Rolled out http://trac.webkit.org/changeset/143885)
Comment 11 Takashi Sakamoto 2013-02-28 10:37:37 PST
Created attachment 190754 [details]
Patch(revised)
Comment 12 Takashi Sakamoto 2013-02-28 10:42:11 PST
I revised the patch (State& -> const State&) and measured its performance:

https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dGRtWjlFejIyS1BBamtEcElzM1ZidGc&usp=sharing

The spreadsheet compares master's html5-full-render test with patched one's html5-full-render test, i.e.

for both master and patched DumpRenderTree, run xvfb-run out/Release/DumpRenderTree third_party/WebKit/PerformanceTests/Parser/html5-full-render.html --no-timeout

The result looks almost the same performance. So can I land this patch?
Comment 13 WebKit Review Bot 2013-02-28 11:19:51 PST
Comment on attachment 190754 [details]
Patch(revised)

Attachment 190754 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16847211

New failing tests:
css2.1/t1202-counter-01-b.html
compositing/rtl/rtl-absolute-overflow.html
css1/box_properties/margin_right.html
accessibility/render-counter-text.html
css1/box_properties/margin.html
compositing/rtl/rtl-fixed-overflow.html
http/tests/cache/subresource-failover-to-network.html
css2.1/t1202-counter-02-b.html
compositing/iframes/iframe-in-composited-layer.html
accessibility/first-letter-text-transform-causes-crash.html
compositing/rtl/rtl-fixed-overflow-scrolled.html
compositing/layer-creation/overlap-transformed-and-clipped.html
compositing/scrollbar-painting.html
compositing/overflow/clip-content-under-overflow-controls.html
compositing/generated-content.html
compositing/overflow/remove-overflow-crash2.html
compositing/overflow/content-loses-scrollbars.html
css2.1/t1202-counter-03-b.html
compositing/reflections/nested-reflection-on-overflow.html
css1/pseudo/multiple_pseudo_elements.html
css1/pseudo/firstline.html
css1/pseudo/firstletter.html
compositing/overflow/scrollbar-painting.html
css1/color_and_background/background_attachment.html
compositing/overflow/overflow-scroll.html
compositing/rtl/rtl-absolute-overflow-scrolled.html
css1/color_and_background/background_repeat.html
css1/pseudo/pseudo_elements_in_selectors.html
css2.1/t0510-c25-pseudo-elmnt-00-c.html
css2.1/t1202-counter-00-b.html
Comment 14 Build Bot 2013-02-28 11:33:38 PST
Comment on attachment 190754 [details]
Patch(revised)

Attachment 190754 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16758390

New failing tests:
css2.1/t1202-counter-01-b.html
css2.1/t1202-counter-08-b.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counter-07-b.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
css2.1/t1202-counter-15-b.html
compositing/generated-content.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
http/tests/cookies/third-party-cookie-relaxing.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css2.1/t1202-counters-01-b.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counter-03-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html
Comment 15 Ryosuke Niwa 2013-02-28 11:40:52 PST
(In reply to comment #12)
> I revised the patch (State& -> const State&) and measured its performance:
> 
> https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dGRtWjlFejIyS1BBamtEcElzM1ZidGc&usp=sharing

FYI, http://trac.webkit.org/wiki/Performance%20Tests#AggregatingandComparingResults

> The spreadsheet compares master's html5-full-render test with patched one's html5-full-render test, i.e.
> for both master and patched DumpRenderTree, run xvfb-run out/Release/DumpRenderTree third_party/WebKit/PerformanceTests/Parser/html5-full-render.html --no-timeout
> 
> The result looks almost the same performance. So can I land this patch?

Do you observe a difference when you run the baseline and the one with the old patch? i.e. Can you reproduce the perf. regression we saw on the bots on your machine?
Comment 16 Build Bot 2013-02-28 12:52:24 PST
Comment on attachment 190754 [details]
Patch(revised)

Attachment 190754 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16758428

New failing tests:
css2.1/t1202-counter-01-b.html
css2.1/t1202-counter-08-b.html
css2.1/t1202-counters-06-b.html
css2.1/t1202-counter-07-b.html
css2.1/t1202-counter-02-b.html
css2.1/t1202-counter-04-b.html
css2.1/t1202-counters-12-b.html
css2.1/t1202-counter-15-b.html
css2.1/t1202-counters-09-b.html
compositing/generated-content.html
css2.1/t1202-counters-11-b.html
css2.1/t1202-counter-16-f.html
css2.1/t1202-counters-04-b.html
css2.1/t1202-counters-03-b.html
css2.1/t1202-counter-12-b.html
css2.1/t1202-counters-07-b.html
http/tests/cookies/third-party-cookie-relaxing.html
css2.1/t1202-counter-09-b.html
css2.1/t1202-counters-08-b.html
css2.1/t1202-counter-14-b.html
css2.1/t1202-counter-11-b.html
css2.1/t1202-counters-01-b.html
css2.1/t1202-counter-13-b.html
css2.1/t1202-counter-05-b.html
css2.1/t1202-counter-06-b.html
css2.1/t1202-counter-03-b.html
css2.1/t1202-counters-02-b.html
css2.1/t1202-counters-00-b.html
css2.1/t1202-counter-00-b.html
css2.1/t1202-counters-05-b.html
Comment 17 Hajime Morrita 2013-03-04 16:59:48 PST
(In reply to comment #16)
> New failing tests:
> css2.1/t1202-counter-01-b.html
> css2.1/t1202-counter-08-b.html
> css2.1/t1202-counters-06-b.html
> css2.1/t1202-counter-07-b.html
> css2.1/t1202-counter-02-b.html
> css2.1/t1202-counter-04-b.html
> css2.1/t1202-counters-12-b.html
> css2.1/t1202-counter-15-b.html
> css2.1/t1202-counters-09-b.html
> compositing/generated-content.html
> css2.1/t1202-counters-11-b.html
> css2.1/t1202-counter-16-f.html
> css2.1/t1202-counters-04-b.html
> css2.1/t1202-counters-03-b.html
> css2.1/t1202-counter-12-b.html
> css2.1/t1202-counters-07-b.html
> http/tests/cookies/third-party-cookie-relaxing.html
> css2.1/t1202-counter-09-b.html
> css2.1/t1202-counters-08-b.html
> css2.1/t1202-counter-14-b.html
> css2.1/t1202-counter-11-b.html
> css2.1/t1202-counters-01-b.html
> css2.1/t1202-counter-13-b.html
> css2.1/t1202-counter-05-b.html
> css2.1/t1202-counter-06-b.html
> css2.1/t1202-counter-03-b.html
> css2.1/t1202-counters-02-b.html
> css2.1/t1202-counters-00-b.html
> css2.1/t1202-counter-00-b.html
> css2.1/t1202-counters-05-b.html

Are these failures real?
Comment 18 Takashi Sakamoto 2013-03-04 21:22:45 PST
Created attachment 191396 [details]
Patch
Comment 19 Takashi Sakamoto 2013-03-04 22:19:46 PST
I rebase-lined and fixed cq- errors.

(In reply to comment #15)
> (In reply to comment #12)
> > I revised the patch (State& -> const State&) and measured its performance:

> Do you observe a difference when you run the baseline and the one with the old patch? i.e. Can you reproduce the perf. regression we saw on the bots on your machine?

I compared master build with patched build again. The result was:

https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dGUwbWNEZm1iREQxNHE5WTl1dkVIMGc&usp=sharing

My patch seems to cause about 1% performance regression. I think, this is almost the same performance regression as the old patch.