Issue metadata
Sign in to add a comment
|
DOM API for getting elements are slow compared to version 48
Reported by
dawild...@gmail.com,
Mar 17 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36 Steps to reproduce the problem: 1. open https://jsperf.com/jquery-selectors-vs-native-api/23 in chrome 49 2. open https://jsperf.com/jquery-selectors-vs-native-api/23 in chrom 48 3. open https://jsperf.com/jquery-selectors-vs-native-api/23 in chrome 47 What is the expected behavior? version 47, 48 used to show performance of document.getElementById around 26M ops/sec. What went wrong? version 49 shows performance of document.getElementById around 5M ops/sec. querySelector, querySelectorAll, getElementsByClassName all are slowed compared to previous chrome. Did this work before? Yes Previous versions. Chrome version: 49.0.2623.87 Channel: stable OS Version: OS X 10.11.3 Flash Version: Shockwave Flash 21.0 r0 I don't know it's a problem of a test model in jsperf or chrome, but something has happend. 5M ops/sec for getElementsById is not bad, but used to achieve 26~27M ops/sec.
,
Mar 18 2016
Able to reproduce the above issue on All-OS(Windows, Mac 10.11.3 & Ubuntu 14.04) with chrome version '49.0.2623.87'. This issue is broken in M49 build, below is the bisect details. Manual bisect: ------------- Last known Good build: 49.0.2588.0 — 27 Million First known BAD build: 49.0.2589.0 — 3.72 Million Below is the narrow bisect: -------------------------- You are probably looking for a change made after 364656 (known good), but no lat er than 364668 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/b1d0baa16833f6eadbf0f73b41d38758a65e6bdd..dc6e70e7c9bb9341fe351f2bfcabdbb24fa62a83 From the above narrow bisect plausible offending CL:https://chromium.googlesource.com/chromium/src/+/37ec8f49dfa32250bf46c8e3e91833e599e93655 ? @jochen:Hey, would you mind checking the above issue and see if it's related to your change ? Feel free to re-assign to concern owner if you are not the correct one. Thank you!
,
Mar 18 2016
thx for the bisect, will address shortly Toon, it looks like we don't install a setter, but the attribute is installed as DontDelete, so I guess CreateDataProperty just fails silently and we're stuck with the getter. I guess I'll just not install a getter to begin with...
,
Mar 18 2016
,
Mar 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/2a1570ef1a92f2622418330a1e088f0a5d869fed commit 2a1570ef1a92f2622418330a1e088f0a5d869fed Author: jochen <jochen@chromium.org> Date: Mon Mar 21 07:52:04 2016 Temporarily undeprecate ForceSet I first need to figure out what to do about window.document before we can deprecate this: window.document is a regular accessor, however, once the window navigated from about:blank, its value will never change. Blink uses ForceSet to then replace the accessor with a data constant which has way better performance than invoking the accessor all the time. Since the accessor, however, is installed as read only & non-configurable, there is no spec compliant way to pull this off right now BUG= chromium:595601 R=verwaest@chromium.org LOG=y Review URL: https://codereview.chromium.org/1816033002 Cr-Commit-Position: refs/heads/master@{#34919} [modify] https://crrev.com/2a1570ef1a92f2622418330a1e088f0a5d869fed/include/v8.h
,
Mar 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b8fd0235bcbb1088d9a3660d581ebfdc161f953 commit 3b8fd0235bcbb1088d9a3660d581ebfdc161f953 Author: jochen <jochen@chromium.org> Date: Mon Mar 21 15:25:36 2016 Revert to using ForceSet for installing the document property for now Depends on https://codereview.chromium.org/1816033002 BUG= 595601 R=verwaest@chromium.org Review URL: https://codereview.chromium.org/1817003002 Cr-Commit-Position: refs/heads/master@{#382292} [modify] https://crrev.com/3b8fd0235bcbb1088d9a3660d581ebfdc161f953/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
,
Mar 21 2016
tiny change to address performance regression. Would like to merge back to M49 after a day or two of canary coverage.
,
Mar 21 2016
Can you add some tests to prevent this sort of thing from going unnoticed in the future?
,
Mar 22 2016
Let me know how things look on canary. What's the long term plan here? Seems like we undeprecated ForceSet to work around this issue.
,
Mar 22 2016
The effect of ForceSet we rely on is to indicate that the getter() will always return the same value from this point on, so it can be treated as a constant. We'll need to introduce an API to accomplish this without relying on ForceSet (which also changes the getter to a value which in turn is a spec violation) looking good on canary btw
,
Mar 22 2016
[Automated comment] Request affecting a post-stable build (M49), manual review required.
,
Mar 22 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Mar 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/a48016023552db4c0db7f41db518967b5cba5e0f commit a48016023552db4c0db7f41db518967b5cba5e0f Author: Jochen Eisinger <jochen@chromium.org> Date: Tue Mar 22 15:26:11 2016 Version 5.0.71.25 (cherry-pick) Merged 2a1570ef1a92f2622418330a1e088f0a5d869fed Temporarily undeprecate ForceSet BUG= chromium:595601 LOG=N TBR=hablich@chromium.org Review URL: https://codereview.chromium.org/1824793003 . Cr-Commit-Position: refs/branch-heads/5.0@{#32} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} [modify] https://crrev.com/a48016023552db4c0db7f41db518967b5cba5e0f/include/v8-version.h [modify] https://crrev.com/a48016023552db4c0db7f41db518967b5cba5e0f/include/v8.h
,
Mar 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83b65358ed3604af749d8cf375942a8239b25e09 commit 83b65358ed3604af749d8cf375942a8239b25e09 Author: Jochen Eisinger <jochen@chromium.org> Date: Tue Mar 22 15:32:35 2016 Revert to using ForceSet for installing the document property for now Depends on https://codereview.chromium.org/1816033002 BUG= 595601 R=verwaest@chromium.org Review URL: https://codereview.chromium.org/1817003002 Cr-Commit-Position: refs/heads/master@{#382292} (cherry picked from commit 3b8fd0235bcbb1088d9a3660d581ebfdc161f953) Review URL: https://codereview.chromium.org/1825493003 . Cr-Commit-Position: refs/branch-heads/2661@{#341} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} [modify] https://crrev.com/83b65358ed3604af749d8cf375942a8239b25e09/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
,
Mar 22 2016
Thanks, c#10. Merge approved for M49 (branch 2623). Please merge your change in ASAP, we are planning to cut a stable candidate at 5PM PST today.
,
Mar 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/1c998eae01e53610a852e6b2d9b7d2822eefe8f3 commit 1c998eae01e53610a852e6b2d9b7d2822eefe8f3 Author: Jochen Eisinger <jochen@chromium.org> Date: Tue Mar 22 18:55:25 2016 Version 4.9.385.33 (cherry-pick) Merged 2a1570ef1a92f2622418330a1e088f0a5d869fed Temporarily undeprecate ForceSet BUG= chromium:595601 LOG=N R=hablich@chromium.org Review URL: https://codereview.chromium.org/1827533002 . Cr-Commit-Position: refs/branch-heads/4.9@{#39} Cr-Branched-From: 2fea296569597e5064f81fd8fce58f1848de261a-refs/heads/4.9.385@{#1} Cr-Branched-From: 0c1430ac2b65847559d6a09f883ee7e5a91063c9-refs/heads/master@{#33306} [modify] https://crrev.com/1c998eae01e53610a852e6b2d9b7d2822eefe8f3/include/v8-version.h [modify] https://crrev.com/1c998eae01e53610a852e6b2d9b7d2822eefe8f3/include/v8.h
,
Mar 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c6c752ea00236f2438f08ff7370459b67d831eee commit c6c752ea00236f2438f08ff7370459b67d831eee Author: Jochen Eisinger <jochen@chromium.org> Date: Tue Mar 22 18:59:30 2016 Revert to using ForceSet for installing the document property for now Depends on https://codereview.chromium.org/1816033002 BUG= 595601 R=verwaest@chromium.org Review URL: https://codereview.chromium.org/1817003002 Cr-Commit-Position: refs/heads/master@{#382292} (cherry picked from commit 3b8fd0235bcbb1088d9a3660d581ebfdc161f953) Review URL: https://codereview.chromium.org/1818023004 . Cr-Commit-Position: refs/branch-heads/2623@{#647} Cr-Branched-From: 92d77538a86529ca35f9220bd3cd512cbea1f086-refs/heads/master@{#369907} [modify] https://crrev.com/c6c752ea00236f2438f08ff7370459b67d831eee/third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp
,
Mar 23 2016
Verified the fix on Latest Beta#50.0.2661.49 for Win7 64-bit, Mac OS X 10.11.3 & Linux Ubuntu 14.04 - Seems to be fix is WAI. Win7 64-bit OS: document.getElementById is around 26M ops/sec. Mac OS X 10.11.3: document.getElementById is around 22M ops/sec. Linux Ubuntu 14.04: document.getElementById is around 38M ops/sec. Thank you! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by tkent@chromium.org
, Mar 17 2016Labels: Needs-Bisect