Issue metadata
Sign in to add a comment
|
1.1%-193.8% regression in memory.top_10_mobile_stress at 430543:430583 |
||||||||||||||||||||||
Issue description
,
Nov 9 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8996445042989141760
,
Nov 9 2016
Issue 663820 has been merged into this issue.
,
Nov 9 2016
=== Auto-CCing suspected CL author hiroshige@chromium.org === Hi hiroshige@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Reland of Remove EventSender from HTMLStyleElement Author : hiroshige Commit description: This CL replaces EventSender in HTMLLinkElement with postTask(): Instead of calling HTMLStyleElement::dispatchPendingLoadEvents() in Document::implicitClose() to enforce <style>'s onload events to be executed before the document's onload event, this CL blocks document's onload until <style>'s onload event using IncrementLoadEventDelayCount. This CL changes the timing of body onload, particularly causes some of the following events that were previously called AFTER document onload to be called BEFORE document onload: [A] The first layout. [B] Latter half of FrameLoader::finishedParsing(). [C] JavaScript function |f| scheduled as |setTimeout(f, 0)| in the <script> at the end of <body>. Notable test fixes (marked with the causes of failures): fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html: [A] Marked as flaky, because it depends on whether body onload is called before (Pass) or after (Failure) the first layout (crbug/651343). fast/scrolling/scrollbar-tickmarks-styled.html: [A] Fixed by https://codereview.chromium.org/2450793002. fast/scrolling/scrollbar-tickmarks-hittest.html: [A] Fixed by https://codereview.chromium.org/2484613003. fast/forms/text/text-set-selection-crash.html: [A] There is race between "Blink Test Plugin: initializing" console log (which is asynchronously triggered by the first layout) and document onload (=test finish). Because the test should pass if not crashes, this CL make the test to ignore the log, by converting the test to use testharness.js and removing the -expected.txt. virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html: [B] Fixed by https://codereview.chromium.org/2467433002. svg/animations/animate-end-attribute-numeric-precision.html: [C] After this CL, calling executeTest() in svg/animations/script-tests/animate-end-attribute-numeric-precision.js causes sampleAnimation() to be called before document onload and test to fail. We set |animationStartsImmediately| true instead, in order to ensure the test is started in document onload. fast/harness/error-in-async-test.html: [C] Whether "Uncaught Exception" console log is shown depends on whether buggyAsyncCode() is executed before or after document onload. This CL deflakes the test by ensuring it is executed after document onload. accessibility/inline-text-changes.html: [Race between the accessibility notification and document onload] Ignore the second call of the notification handler. Even before this CL, the notification is called multiple times, but the first notification terminated the test immediately by finishJSTest() because it is called after document onload. This CL makes the notification calls before document onload, and thus the test continues until document onload and we have to ignore the second call explicitly. fast/block/float/marquee-shrink-to-avoid-floats.html: The layout tree changes slightly, but it looks like acceptable because the image expectation matches. BUG= 624697 , 651343 Committed: https://crrev.com/52d00fd56184deadce99ff4fe599156ea1da3543 Review-Url: https://codereview.chromium.org/2269043002 Cr-Original-Commit-Position: refs/heads/master@{#430348} Cr-Commit-Position: refs/heads/master@{#430572} Commit : b8cb16f79a95c8bc639a9c58785a8484c197aca9 Date : Tue Nov 08 10:55:53 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@430563 7611341 4028.27 5 good chromium@430568 7611929 2262.28 5 good chromium@430570 7611360 4052.41 5 good chromium@430571 7823382 467264 5 good chromium@430572 31012926 3302185 5 bad <-- Bisect job ran on: android_nexus6_perf_bisect Bug ID: 663823 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests memory.top_10_mobile_stress Test Metric: memory:chrome:all_processes:reported_by_chrome:gpu:effective_size_avg/foreground/https_www_google_co_uk_hl_en_q_science Relative Change: 307.46% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus6_perf_bisect/builds/2735 Job details: https://chromeperf.appspot.com/buildbucket_job_status/8996445042989141760 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5788651332567040 | 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 Tests>AutoBisect. Thank you!
,
Nov 17 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8995770497923506976
,
Nov 17 2016
===== BISECT JOB RESULTS =====
Status: completed
===== SUSPECTED CL(s) =====
Subject : Reland of Remove EventSender from HTMLStyleElement
Author : hiroshige
Commit description:
This CL replaces EventSender in HTMLLinkElement with postTask():
Instead of calling HTMLStyleElement::dispatchPendingLoadEvents() in
Document::implicitClose() to enforce <style>'s onload events to be executed
before the document's onload event, this CL blocks document's onload until
<style>'s onload event using IncrementLoadEventDelayCount.
This CL changes the timing of body onload, particularly causes some of the
following events that were previously called AFTER document onload
to be called BEFORE document onload:
[A] The first layout.
[B] Latter half of FrameLoader::finishedParsing().
[C] JavaScript function |f| scheduled as |setTimeout(f, 0)| in the <script>
at the end of <body>.
Notable test fixes (marked with the causes of failures):
fast/text/international/inline-plaintext-relayout-with-leading-neutrals.html:
[A] Marked as flaky, because it depends on whether body onload is called
before (Pass) or after (Failure) the first layout (crbug/651343).
fast/scrolling/scrollbar-tickmarks-styled.html:
[A] Fixed by https://codereview.chromium.org/2450793002.
fast/scrolling/scrollbar-tickmarks-hittest.html:
[A] Fixed by https://codereview.chromium.org/2484613003.
fast/forms/text/text-set-selection-crash.html:
[A] There is race between "Blink Test Plugin: initializing" console log
(which is asynchronously triggered by the first layout) and document onload
(=test finish). Because the test should pass if not crashes, this CL make
the test to ignore the log, by converting the test to use testharness.js
and removing the -expected.txt.
virtual/rootlayerscrolls/fast/history/scroll-restoration/scroll-restoration-fragment-navigation-crossdoc.html:
[B] Fixed by https://codereview.chromium.org/2467433002.
svg/animations/animate-end-attribute-numeric-precision.html:
[C] After this CL, calling executeTest() in
svg/animations/script-tests/animate-end-attribute-numeric-precision.js
causes sampleAnimation() to be called before document onload and test to fail.
We set |animationStartsImmediately| true instead, in order to ensure the test
is started in document onload.
fast/harness/error-in-async-test.html:
[C] Whether "Uncaught Exception" console log is shown depends on whether
buggyAsyncCode() is executed before or after document onload. This CL
deflakes the test by ensuring it is executed after document onload.
accessibility/inline-text-changes.html:
[Race between the accessibility notification and document onload]
Ignore the second call of the notification handler.
Even before this CL, the notification is called multiple times, but the first
notification terminated the test immediately by finishJSTest() because it is
called after document onload.
This CL makes the notification calls before document onload, and thus the test
continues until document onload and we have to ignore the second call
explicitly.
fast/block/float/marquee-shrink-to-avoid-floats.html:
The layout tree changes slightly, but it looks like acceptable because the
image expectation matches.
BUG= 624697 , 651343
Committed: https://crrev.com/52d00fd56184deadce99ff4fe599156ea1da3543
Review-Url: https://codereview.chromium.org/2269043002
Cr-Original-Commit-Position: refs/heads/master@{#430348}
Cr-Commit-Position: refs/heads/master@{#430572}
Commit : b8cb16f79a95c8bc639a9c58785a8484c197aca9
Date : Tue Nov 08 10:55:53 2016
===== TESTED REVISIONS =====
Revision Mean Std Dev N Good?
chromium@430558 4385534 514146 12 good
chromium@430567 4099188 1608637 8 good
chromium@430571 4127467 1829348 12 good
chromium@430572 12272966 466424 5 bad <--
chromium@430573 10693237 11764432 12 bad
chromium@430575 11215665 13263116 12 bad
chromium@430590 12137496 605300 8 bad
Bisect job ran on: android_nexus7_perf_bisect
Bug ID: 663823
Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=https.www.google.co.uk.hl.en.q.science memory.top_10_mobile_stress
Test Metric: memory:chrome:all_processes:reported_by_chrome:v8:effective_size_avg/foreground/https_www_google_co_uk_hl_en_q_science
Relative Change: 179.39%
Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_perf_bisect/builds/3490
Job details: https://chromeperf.appspot.com/buildbucket_job_status/8995770497923506976
Not what you expected? We'll investigate and get back to you!
https://chromeperf.appspot.com/bad_bisect?try_job_id=4980418732883968
| 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 Tests>AutoBisect. Thank you!
,
Nov 17 2016
My CL is unlikely to directly impact v8 memory usage, but as the regression is clear and large, I'm investigating why. My CL affects document onload timing and I suspect my CL affected garbage collection schedule.
,
Dec 27 2016
I still don't know why, but calling checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent() resolved the memory consumption increase. (https://codereview.chromium.org/2511663002/ Patch Set 4) Similar hack resolved another performance issue (not committed though) in the case of HTMLLinkElement (https://codereview.chromium.org/2391923002/). Perhaps we should implement the hack in all ex-EventSender code, preferably in more systematic way.
,
Jan 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0712ce38d1a05590e731d07f9aad4b46870f53ae commit 0712ce38d1a05590e731d07f9aad4b46870f53ae Author: hiroshige <hiroshige@chromium.org> Date: Thu Jan 19 23:23:09 2017 Call checkCompleted() synchronously in HTMLStyleElement::dispatchPendingEvent() Previously, there was an asynchronous hop from <style>'s load event to document's load event, because HTMLStyleElement::dispatchPendingEvent() scheduled checkCompleted() asynchronously via IncrementLoadEventDelayCount. This CL makes HTMLStyleElement::dispatchPendingEvent() to call checkCompleted() synchronously, by using IncrementLoadEventDelayCount::clearAndCheckLoadEvent(). Because checkCompleted() is called at the end of asynchronously called HTMLStyleElement::dispatchPendingEvent(), this CL doesn't increase synchronous timing dependencies around Document load events. This fixes performance (memory consumption) regression in memory.top_10_mobile_stress caused by https://codereview.chromium.org/2275493002/. BUG= 663823 , 624697 TEST=https_www_google_co_uk_hl_en_q_science in memory:chrome:all_processes:reported_by_chrome:v8:effective_size in memory.top_10_mobile_stress Review-Url: https://codereview.chromium.org/2605173002 Cr-Commit-Position: refs/heads/master@{#444886} [modify] https://crrev.com/0712ce38d1a05590e731d07f9aad4b46870f53ae/third_party/WebKit/LayoutTests/fast/dom/HTMLDialogElement/inert-node-is-unfocusable-expected.txt [modify] https://crrev.com/0712ce38d1a05590e731d07f9aad4b46870f53ae/third_party/WebKit/LayoutTests/http/tests/loading/preload-img-test-expected.txt [modify] https://crrev.com/0712ce38d1a05590e731d07f9aad4b46870f53ae/third_party/WebKit/LayoutTests/plugins/overlay-scrollbar-mouse-capture-expected.txt [modify] https://crrev.com/0712ce38d1a05590e731d07f9aad4b46870f53ae/third_party/WebKit/Source/core/html/HTMLStyleElement.cpp [modify] https://crrev.com/0712ce38d1a05590e731d07f9aad4b46870f53ae/third_party/WebKit/Source/core/html/HTMLStyleElement.h
,
Feb 21 2017
,
Mar 14 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by rsch...@chromium.org
, Nov 9 2016