Bug 109909

Summary:

[Refactoring] Make m_state an on-stack object

Product:

WebKit

Reporter:

Takashi Sakamoto <tasak>

Component:

CSS

Assignee:

Takashi Sakamoto <tasak>

Status:

RESOLVED CONFIGURATION CHANGED

Severity:

Normal

CC:

allan.jensen, bfulgham, buildbot, dglazkov, esprehn+autocc, jyasskin, koivisto, macpherson, menard, morrita, ojan.autocc, rniwa, webcomponents-bugzilla, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

110719

Bug Blocks:

89879

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none
Patch(revised)	none
Patch	none

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

Antti Koivisto

Comment 7 2013-02-24 02:34:15 PST

This looks like 2-3% performance regression in Parser/html5-full-render: https://perf.webkit.org/#mode=charts&days=56&chartList=%5B%5B%22Mac%20Lion%22%2C%22Parser%2Fhtml5-full-render%3ATime%22%5D%5D&zoom=%5B1361218175932.3066%2C1361543074858.7913%5D

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.