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

Issue 94403002: Move popular DOM attributes to prorotype chains (Closed)

Created:
7 years ago by haraken
Modified:
7 years ago
CC:
blink-reviews, arv+blink, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, watchdog-blink-watchlist_google.com, Inactive
Visibility:
Public.

Description

Move popular DOM attributes to prorotype 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 BUG=43394 This CL moves DOM attributes used in Dromaeo to prototype chains. As explained in the document, this CL is going to regress performance of Dromaeo by 8.7%. Dromaeo/dom-attr 1134 runs/sec => 1145 runs/sec (+1.0%) Dromaeo/dom-modify 488 runs/sec => 493 runs/sec (+1.0%) Dromaeo/dom-query 22860 runs/sec => 21772 runs/sec (-5.0%) Dromaeo/dom-traverse 523 runs/sec => 481 runs/sec (-8.7%) See the design document and the Intent-to-ship email for justification for the regression. 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 that the regression is unacceptable, I'll revert the CL. Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=163043

Patch Set 1 #

Patch Set 2 : #

Total comments: 2

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -12 lines) Patch
M LayoutTests/inspector/console/console-dir-expected.txt View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/Document.idl View 1 2 1 chunk +1 line, -1 line 0 comments Download
M Source/core/dom/Element.idl View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/dom/Node.idl View 1 2 1 chunk +7 lines, -7 lines 0 comments Download
M Source/core/dom/NodeList.idl View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 12 (0 generated)
haraken
PTAL
7 years ago (2013-11-29 04:28:54 UTC) #1
haraken
https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt File LayoutTests/inspector/console/console-dir-expected.txt (right): https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt#newcode17 LayoutTests/inspector/console/console-dir-expected.txt:17: length: (...) This change looks strange. Looking.
7 years ago (2013-11-29 04:39:39 UTC) #2
haraken
https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt File LayoutTests/inspector/console/console-dir-expected.txt (right): https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt#newcode17 LayoutTests/inspector/console/console-dir-expected.txt:17: length: (...) On 2013/11/29 04:39:40, haraken wrote: > > ...
7 years ago (2013-11-29 05:19:49 UTC) #3
pfeldman
On 2013/11/29 05:19:49, haraken wrote: > https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt > File LayoutTests/inspector/console/console-dir-expected.txt (right): > > https://codereview.chromium.org/94403002/diff/20001/LayoutTests/inspector/console/console-dir-expected.txt#newcode17 > ...
7 years ago (2013-11-29 12:03:34 UTC) #4
haraken
> > pfeldman: What do you think? > > Sounds good. Do you have an ...
7 years ago (2013-11-29 12:28:00 UTC) #5
jochen (gone - plz use gerrit)
lgtm
7 years ago (2013-12-02 10:37:38 UTC) #6
haraken
On 2013/12/02 10:37:38, jochen wrote: > lgtm Thanks, let me ping chrome-sheriff and land it ...
7 years ago (2013-12-02 10:39:45 UTC) #7
pfeldman
On 2013/11/29 12:28:00, haraken wrote: > > > pfeldman: What do you think? > > ...
7 years ago (2013-12-02 12:22:59 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haraken@chromium.org/94403002/20001
7 years ago (2013-12-03 00:12:34 UTC) #9
commit-bot: I haz the power
Change committed as 163043
7 years ago (2013-12-03 01:09:13 UTC) #10
trchen
A revert of this CL has been created in https://codereview.chromium.org/103673003/ by trchen@chromium.org. The reason for ...
7 years ago (2013-12-04 04:45:41 UTC) #11
haraken
7 years ago (2013-12-04 04:47:14 UTC) #12
Message was sent while issue was closed.
On 2013/12/04 04:45:41, trchen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/103673003/ by mailto:trchen@chromium.org.
> 
> The reason for reverting is: Breaks internal Android instrumentation bots
> (innerHTML setter crash).

trchen: Sorry, this CL was already reverted in r163107.

Powered by Google App Engine
This is Rietveld 408576698