New issue
Advanced search Search tips

Issue 729229 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.4%-23.7% regression in system_health.memory_mobile at 476026:476274

Project Member Reported by m...@chromium.org, Jun 2 2017

Issue description

See the link to graphs below.
 
Cc: aboxhall@chromium.org
Owner: aboxhall@chromium.org

=== Auto-CCing suspected CL author aboxhall@chromium.org ===

Hi aboxhall@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 : aboxhall
  Commit : 6281b70b9a36a64590f99c799ee55264da6cb52d
  Date   : Thu Jun 01 10:09:50 2017
  Subject: Check whether a text node may be about to be redistributed in ShouldUpdateLayoutByReattaching(). Prevents use-after-free when SetTextWithOffset is called with dirty distribution.

Bisect Details
  Configuration: android_webview_arm64_aosp_perf_bisect
  Benchmark    : system_health.memory_mobile
  Metric       : memory:webview:all_processes:reported_by_os:system_memory:java_heap:proportional_resident_size_avg/load_media/load_media_dailymotion
  Change       : 2.32% | 5070506.66667 -> 5187925.33333

Revision             Result                  N
chromium@476241      5070507 +- 51945.5      6      good
chromium@476242      5649408 +- 153886       6      bad       <--
chromium@476243      5585920 +- 48809.5      6      bad
chromium@476244      5454165 +- 113721       6      bad
chromium@476247      5323776 +- 97274.6      6      bad
chromium@476253      5187925 +- 104655       6      bad

Please refer to the following doc on diagnosing memory regressions:
  https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-webview --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=load.media.dailymotion system_health.memory_mobile

Debug Info
  https://chromeperf.appspot.com/buildbucket_job_status/8977856648525847696

Is this bisect wrong?
  https://chromeperf.appspot.com/bad_bisect?try_job_id=4901431699570688


| 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!
This makes a lot of sense. Looking at a fix.

Comment 5 by m...@chromium.org, Jun 5 2017

Cc: -miu@google.com
Friendly perf sheriff ping, any update on this?
Apologies for the silence. Have been looking at this but it's quite complicated to fix, unfortunately.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 26 2017

Labels: merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/303e0e83015674dec5bf2fc1a5eafe2871e867ee

commit 303e0e83015674dec5bf2fc1a5eafe2871e867ee
Author: Takayoshi Kochi <kochi@chromium.org>
Date: Mon Jun 26 04:55:46 2017

Only call TextChanged on existing AXObjects. This avoids triggering the code which calls UpdateDistribution as part of creating a new AXObject, which was causing issues.

This change should cause no behaviour changes, as in cases where an AXObject needs to be created for a text node, its parent will also have a ChildrenChanged notification fired on it, which will cause the appropriate updates to fire.

Reverts https://codereview.chromium.org/2926713002/ as the code which causes UpdateDistribution to be called should never run with this change, so we don't need to check whether distribution is dirty any more.

BUG= 729229 , 732200 , 734348 

Review-Url: https://codereview.chromium.org/2939933002
Cr-Original-Commit-Position: refs/heads/master@{#480792}
Review-Url: https://codereview.chromium.org/2957733002 .
Cr-Commit-Position: refs/branch-heads/3112@{#464}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}

[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/content/test/data/accessibility/event/menulist-collapse-expected-win.txt
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/LayoutTests/accessibility/contenteditable-notifications.html
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/LayoutTests/accessibility/presentation-owned-elements.html
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/LayoutTests/resources/accessibility-helper.js
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/core/layout/LayoutMenuList.cpp
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/modules/accessibility/AXMenuList.cpp
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/modules/accessibility/AXMenuListPopup.cpp
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/modules/accessibility/AXMenuListPopup.h
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp
[modify] https://crrev.com/303e0e83015674dec5bf2fc1a5eafe2871e867ee/third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h

Status: Assigned (was: Untriaged)
Explictly assigning. A CL you landed tripped one of the speed metrics we measure in the lab. If this is the first time this has happened to one of your CLs, or if it's been a while, please read: https://chromium.googlesource.com/chromium/src/+/master/docs/speed/addressing_performance_regressions.md

We're looking for one of the following:
1. Justification via explanation
2. Plan to revert or fix
3. Angry rage throwing of equipment at my head

Just be aware that I'm trained in trumpet playing and First Aid and am not afraid to use it.

Note: This was a bulk edit message and not very personal.
Status: Fixed (was: Assigned)
This seems to be fixed.

Sign in to add a comment