Speedometer regression by 35 % |
||||
Issue descriptionSee https://chromeperf.appspot.com/report?sid=e21c2f80d9c498fd8143316a1023c3895a0baac98bd214449175b59223a42bcf Bisect https://build.chromium.org/p/tryserver.chromium.perf/builders/win_perf_bisect/builds/7142 points to https://codereview.chromium.org/2680943004 which makes sense as the same regression could be observed the last time the CL was landed. Can you please revert your CL and reland it with proper fixes?
,
Feb 16 2017
Investigating...
,
Feb 16 2017
Found root cause and fixed it [1]. Reviewing... [1] http://crrev.com/2698793003: Get rid of redundant layout tree update related to selection
,
Feb 16 2017
Great to hear. If this does not stick please revert the original CL. This is currently resulting in Speedometer regression of 35 % over all platforms. Speedometer is quite representative of the real web. Considering that the branch point is "soon", a regression like this should not linger to long on master as it might mask other, smaller regressions.
,
Feb 17 2017
,
Feb 17 2017
Committing[1]... [1] http://crrev.com/2698793003: Get rid of redundant layout tree update related to selection
,
Feb 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8a65d35c402aa21c4ce9b07a63a109d113a6066 commit e8a65d35c402aa21c4ce9b07a63a109d113a6066 Author: yosin <yosin@chromium.org> Date: Fri Feb 17 09:01:40 2017 Get rid of redundant layout tree update related to selection This patch gets rid of redundant layout tree update regarding selection getter to recover execution speed in Speedometer performance test. Since CL[1], |FrameSelection| getters updates layout tree on-demand rather than invalidated values, e.g. |VisibleSelection| can hold invalid positions after style and DOM tree modification. However, following getters in |FrameSeleciton| don't need to clean layout tree: - affinity() - isNone() for |TextControl| - isRange() for |TextControl| - start()/end() for |TextControl| - checking editability base on selection start position; it is enough to check one of base or extent is ediable since selection can not cross editing boundary. [1] crrev.com/2680943004 Make FrameSelection to hold non-canonicalized positions BUG= 692537 TEST=Speedometer performance test Review-Url: https://codereview.chromium.org/2698793003 Cr-Commit-Position: refs/heads/master@{#451270} [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/DOMSelection.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/Editor.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/FrameSelection.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/FrameSelection.h [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/SelectionController.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/SelectionTemplate.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/SelectionTemplate.h [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp [modify] https://crrev.com/e8a65d35c402aa21c4ce9b07a63a109d113a6066/third_party/WebKit/Source/core/html/TextControlElement.cpp
,
Feb 17 2017
Local run of speedometer recovers score by comparing release build. I hope we get same behavior on official build. Since the root cause is redundant updating of style and layout tree, we don't expect big regression anymore.
,
Feb 18 2017
In http://browserbench.org/Speedometer/InteractiveRunner.html, Blink contribution is 12.13% of total CPU time. Function Name Total CPU (%) Self CPU (%) Total CPU (ms) Self CPU (ms) + blink::V8Attr::valueAttributeGetterCallback 0.17 % 0.04 % 9 2 + blink::V8DataTransferItem::getAsStringMethodCallback 0.02 % 0.00 % 1 0 + blink::V8Document::activeElementAttributeGetterCallback 0.04 % 0.00 % 2 0 + blink::V8DOMTokenList::lengthAttributeGetterCallback 0.02 % 0.00 % 1 0 blink::V8Element::classNameAttributeGetterCallback 0.02 % 0.02 % 1 1 + blink::V8Element::classNameAttributeSetterCallback 0.13 % 0.00 % 7 0 + blink::V8Element::idAttributeGetterCallback 0.02 % 0.00 % 1 0 + blink::V8Element::innerHTMLAttributeSetterCallback 9.74 % 0.00 % 508 0 + blink::V8Event::timeStampAttributeGetterCallback 0.08 % 0.00 % 4 0 + blink::V8Gamepad::idAttributeGetterCallback 0.02 % 0.00 % 1 0 + blink::V8HTMLElement::contentEditableAttributeGetterCallback 0.02 % 0.00 % 1 0 + blink::V8HTMLElement::innerTextAttributeSetterCallback 0.15 % 0.00 % 8 0 + blink::V8HTMLElement::styleAttributeGetterCallbackForMainWorld 0.02 % 0.00 % 1 0 + blink::V8HTMLInputElement::checkedAttributeSetterCallback 0.04 % 0.00 % 2 0 + blink::V8HTMLInputElement::valueAttributeGetterCallback 0.15 % 0.00 % 8 0 + blink::V8HTMLInputElement::valueAttributeSetterCallback 0.46 % 0.00 % 24 0 + blink::V8Node::insertBeforeMethodCallbackForMainWorld 0.59 % 0.00 % 31 0 blink::V8Node::nextSiblingAttributeGetterCallbackForMainWorld 0.02 % 0.02 % 1 1 + blink::V8Node::nodeNameAttributeGetterCallback 0.12 % 0.02 % 6 1 + blink::V8Node::nodeTypeAttributeGetterCallback 0.04 % 0.02 % 2 1 + blink::V8Node::nodeValueAttributeSetterCallback 0.06 % 0.00 % 3 0 + blink::V8Node::removeChildMethodCallback 0.06 % 0.00 % 3 0 + blink::V8PerformanceResourceTiming::initiatorTypeAttributeGetterCallback 0.04 % 0.02 % 2 1 + blink::v8SetReturnValueForMainWorld<v8::FunctionCallbackInfo<v8::Value> > 0.10 % 0.04 % 5 2
,
Feb 18 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8987309098386624256
,
Feb 18 2017
On my win10 box: - 56.0.2924.87 (stable) V8 5.6.326.50 158.7 +-1.0 - 58.0.3016.0 (canary) r451403, v8:5.8.239: 161.2 +-3.4
,
Feb 18 2017
=== Auto-CCing suspected CL author yosin@chromium.org === Hi yosin@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : yosin Commit : e8a65d35c402aa21c4ce9b07a63a109d113a6066 Date : Fri Feb 17 09:01:40 2017 Subject: Get rid of redundant layout tree update related to selection Bisect Details Configuration: win_perf_bisect Benchmark : speedometer Metric : Total/http___browserbench.org_Speedometer_ Change : 31.06% | 4780.42458333 -> 3295.62841667 Revision Result N chromium@451268 4780.42 +- 56.3131 6 good chromium@451269 4789.05 +- 64.4792 6 good chromium@451270 3291.34 +- 42.2683 6 bad <-- chromium@451272 3295.63 +- 53.5015 6 bad To Run This Test src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=http...browserbench.org.Speedometer. speedometer Debug Info https://chromeperf.appspot.com/buildbucket_job_status/8987309098386624256 Is this bisect wrong? https://chromeperf.appspot.com/bad_bisect?try_job_id=4548526983348224 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Speed>Bisection. Thank you!
,
Feb 22 2017
,
Mar 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b6b46d096a709930f22c2265b3378930dcf579c3 commit b6b46d096a709930f22c2265b3378930dcf579c3 Author: yoichio <yoichio@chromium.org> Date: Thu Mar 02 05:38:38 2017 Simplify TextControlElementTest.SetSelectionRangeDoesNotCaluseLayout It doesn't make sense that forceLayoutFlag doesn't force layout. Then this CL refine the test that TextControlElement::setSelectionRange doesn't call layout in appropriate condition. BUG= 692537 Review-Url: https://codereview.chromium.org/2716793002 Cr-Commit-Position: refs/heads/master@{#454186} [modify] https://crrev.com/b6b46d096a709930f22c2265b3378930dcf579c3/third_party/WebKit/Source/core/html/TextControlElementTest.cpp |
||||
►
Sign in to add a comment |
||||
Comment 1 by rtoy@chromium.org
, Feb 15 2017