RESOLVED CONFIGURATION CHANGED 109909
[Refactoring] Make m_state an on-stack object 
https://bugs.webkit.org/show_bug.cgi?id=109909
Summary [Refactoring] Make m_state an on-stack object
Takashi Sakamoto
Reported 2013-02-15 00:50:50 PST
Currently m_state is a member variable of StyleResolver. Make the m_state an on-stack object.
Attachments
Patch (233.26 KB, patch)
2013-02-15 01:24 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (233.26 KB, patch)
2013-02-20 00:04 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch for landing (233.24 KB, patch)
2013-02-20 19:15 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch(revised) (234.34 KB, patch)
2013-02-28 10:37 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Patch (234.27 KB, patch)
2013-03-04 21:22 PST, Takashi Sakamoto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Takashi Sakamoto
Comment 1 2013-02-15 01:24:51 PST
Created attachment 188509 [details] Patch
Takashi Sakamoto
Comment 2 2013-02-20 00:04:57 PST
Created attachment 189257 [details] Patch
Antti Koivisto
Comment 3 2013-02-20 05:39:04 PST
Comment on attachment 189257 [details] Patch r=me
Takashi Sakamoto
Comment 4 2013-02-20 19:15:25 PST
Created attachment 189449 [details] Patch for landing
WebKit Review Bot
Comment 5 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>
WebKit Review Bot
Comment 6 2013-02-20 19:41:31 PST
All reviewed patches have been landed. Closing bug.
Hajime Morrita
Comment 8 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.
Antti Koivisto
Comment 9 2013-02-25 03:30:59 PST
Rollout did indeed fix the regression.
Hajime Morrita
Comment 10 2013-02-26 09:42:12 PST
Reopening (Rolled out http://trac.webkit.org/changeset/143885)
Takashi Sakamoto
Comment 11 2013-02-28 10:37:37 PST
Created attachment 190754 [details] Patch(revised)
Takashi Sakamoto
Comment 12 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?
WebKit Review Bot
Comment 13 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
Build Bot
Comment 14 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
Ryosuke Niwa
Comment 15 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?
Build Bot
Comment 16 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
Hajime Morrita
Comment 17 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?
Takashi Sakamoto
Comment 18 2013-03-04 21:22:45 PST
Created attachment 191396 [details] Patch
Takashi Sakamoto
Comment 19 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.
Brent Fulgham
Comment 20 2022-07-13 15:41:46 PDT
This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.
Radar WebKit Bug Importer
Comment 21 2022-07-13 15:42:16 PDT
<rdar://problem/96978025>
Antti Koivisto
Comment 22 2022-07-13 23:39:40 PDT
It has indeed been refactored to be a stack object.
Note You need to log in before you can comment on or make changes to this bug.