Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(551)

Issue 187343006: Revert 168383 "Move popular DOM attributes to prototype chains" (Closed)

Created:
6 years, 9 months ago by haraken
Modified:
6 years, 9 months ago
Reviewers:
haraken
CC:
blink-reviews
Visibility:
Public.

Description

Revert 168383 "Move popular DOM attributes to prototype chains" This CL regressed performance of Dromaeo/dom-traverse unacceptably. https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-mac6%2Cchromium-rel-mac8%2Cchromium-rel-win7-dual%2Clinux-release&tests=dromaeo.domcoretraverse%2Fdom&checked=core > Move popular DOM attributes to prototype chains > > Design document: https://docs.google.com/a/google.com/document/d/1yeHTCHhulVIlrKyx9_gCguAhLfcefVOa9uxxfW2LVG0/edit > > Intent-to-ship-and-implement in blink-dev: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/t0XiZuMey7M/9-5AuhoFyisJ > > This CL moves DOM attributes used in Dromaeo to prototype chains. When I landed the CL two months ago (https://codereview.chromium.org/94403002), it caused 20+% regression in benchmarks. However, now that a V8 side fix is landed, there should be no overhead in moving DOM attributes to prototype chains in theory. > > Dromaeo/dom-attr 1086 runs/sec => 1077 runs/sec (-0.8%) > Dromaeo/dom-modify 522 runs/sec => 525 runs/sec (+0.5%) > Dromaeo/dom-query 26150 runs/sec => 26200 runs/sec (+0.2%) > Dromaeo/dom-traverse 564 runs/sec => 544 runs/sec (-3.6%) > > I don't understand the regression in dom-traverse. The regression is reproducible but I cannot reproduce the regression in a super-micro benchmark that just repeats accessing div.firstChild. Probably the regression might be a noise. > > I'm going to land this CL and see how much regression is observed in perf bots (I'll email to perf-sheriff@). If I find the regression is unacceptable, I'll revert the CL. > > See the design document and the Intent-to-ship email for justification for the regression. > > BUG=43394 > > Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167680 > > R=jochen@chromium.org > > Review URL: https://codereview.chromium.org/158713002 TBR=haraken@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168453

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -10 lines) Patch
M trunk/Source/core/dom/Document.idl View 1 chunk +1 line, -1 line 0 comments Download
M trunk/Source/core/dom/Element.idl View 2 chunks +2 lines, -2 lines 0 comments Download
M trunk/Source/core/dom/Node.idl View 1 chunk +7 lines, -7 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
haraken
6 years, 9 months ago (2014-03-05 08:53:21 UTC) #1
haraken
6 years, 9 months ago (2014-03-05 08:53:31 UTC) #2
Message was sent while issue was closed.
Committed patchset #1 manually as r168453.

Powered by Google App Engine
This is Rietveld 408576698