New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 797423 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

5.2% regression in thread_times.key_idle_power_cases at 525500:525651

Project Member Reported by briander...@chromium.org, Dec 22 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Dec 22 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=797423

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=b3ee879dc655f669d31bc631c6070ded23242532a4e90931c876635edb081419


Bot(s) for this bug's original alert(s):

android-nexus5
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Dec 23 2017

Cc: romax@chromium.org
Owner: romax@chromium.org
Status: Assigned (was: Untriaged)

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

Hi romax@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 : Yafei Duan
  Commit : 528c084044eb174424f183d0df66e577481e39c3
  Date   : Thu Dec 21 04:11:20 2017
  Subject: [Offline Pages] Switching from OPMImpl to OPMTaskified.

Bisect Details
  Configuration: android_nexus5_perf_bisect
  Benchmark    : thread_times.key_idle_power_cases
  Metric       : tasks_per_second_total_all/set-timeout.html (Long Idle)
  Change       : 5.74% | 4.39180815699 -> 4.64391414455

Revision             Result                    N
chromium@525499      4.39181 +- 0.147612       6      good
chromium@525575      4.4144 +- 0.120661        6      good
chromium@525594      4.45761 +- 0.261651       9      good
chromium@525596      4.42495 +- 0.0950014      6      good
chromium@525597      4.6471 +- 0.158578        6      bad       <--
chromium@525599      4.62852 +- 0.146813       6      bad
chromium@525604      4.63625 +- 0.118128       6      bad
chromium@525613      4.63603 +- 0.141931       6      bad
chromium@525651      4.64391 +- 0.144986       6      bad

To Run This Test
  src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=set.timeout.html..Long.Idle. thread_times.key_idle_power_cases

More information on addressing performance regressions:
  http://g.co/ChromePerformanceRegressions

Debug information about this bisect:
  https://chromeperf.appspot.com/buildbucket_job_status/8959470473810452224


For feedback, file a bug with component Speed>Bisection

Comment 4 by carl...@google.com, Dec 23 2017

The identified CL is being reverted: https://crrev.com/c/843717

Comment 5 by carl...@google.com, Dec 23 2017

Cc: carlosk@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4ef2d88a56d67bf788f6c170d9b924f904e84ba9

commit 4ef2d88a56d67bf788f6c170d9b924f904e84ba9
Author: Carlos Knippschild <carlosk@chromium.org>
Date: Sat Dec 23 05:59:54 2017

Revert "[Offline Pages] Switching from OPMImpl to OPMTaskified."

This reverts commit 528c084044eb174424f183d0df66e577481e39c3.

Reason for revert: primary suspect of performance regression reported at  https://crbug.com/797423 .

Original change's description:
> [Offline Pages] Switching from OPMImpl to OPMTaskified.
> 
> Switching OfflinePageModelFactory from creating OfflinePageModelImpl to
> OfflinePageModelTaskified. Also have related changes required in tests.
> 
> 
> Bug:  753595 
> Change-Id: I8de03e1e943cf3e8c3040e5b3572643d72b9fca2
> Reviewed-on: https://chromium-review.googlesource.com/833495
> Commit-Queue: Yafei Duan <romax@chromium.org>
> Reviewed-by: Dmitry Titov <dimich@chromium.org>
> Reviewed-by: Filip Gorski <fgorski@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#525597}

TBR=fgorski@chromium.org,dimich@chromium.org,romax@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  753595 ,  797423 
Change-Id: I3f6b2ddfa2523bd3007a77df2db26bb841729b3e
Reviewed-on: https://chromium-review.googlesource.com/843717
Reviewed-by: Carlos Knippschild <carlosk@chromium.org>
Commit-Queue: Carlos Knippschild <carlosk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526141}
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/chrome/browser/offline_pages/android/offline_page_model_factory.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/chrome/browser/offline_pages/offline_page_request_job_unittest.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/chrome/browser/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/chrome/browser/offline_pages/offline_page_utils_unittest.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/chrome/browser/offline_pages/test_offline_page_model_builder.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/create_archive_task.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/create_archive_task.h
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/create_archive_task_unittest.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/delete_page_task.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/offline_page_model_taskified.cc
[modify] https://crrev.com/4ef2d88a56d67bf788f6c170d9b924f904e84ba9/components/offline_pages/core/model/offline_page_model_taskified.h

Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Dec 24 2017

Issue 797429 has been merged into this issue.
Project Member

Comment 8 by bugdroid1@chromium.org, Jan 3 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d

commit fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d
Author: Yafei Duan <romax@chromium.org>
Date: Wed Jan 03 04:02:41 2018

[Offline Pages] Fix perf regression and crash during model switch.

This CL is depending on the CL of switching OfflinePageModel, and trying
to address the issue of perf regression and increased crash rate during
CreateArchiveTask.

After code review, this CL will merge with 845261 as reland of 833495.

This CL contains:
- Removing CreateArchiveTask and moving all its logic into OPMTaskified.
- Changed when ClearCachedPages is called in OPMTaskified, to make it
  exactly same with OPMImpl to see if this will fix perf regression. If
  not, it might be the issue of database closing.

Bug: 797628,  797423 
Change-Id: Ib810d48637802c82713dfc506d05a14b69110170
Reviewed-on: https://chromium-review.googlesource.com/847684
Commit-Queue: Yafei Duan <romax@chromium.org>
Reviewed-by: Dmitry Titov <dimich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526614}
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/chrome/browser/offline_pages/android/offline_page_model_factory.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/chrome/browser/offline_pages/offline_page_request_job_unittest.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/chrome/browser/offline_pages/offline_page_utils.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/chrome/browser/offline_pages/offline_page_utils_unittest.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/chrome/browser/offline_pages/test_offline_page_model_builder.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/components/offline_pages/core/BUILD.gn
[delete] https://crrev.com/ab8978bb4b2ee8f7bda17209d340ac76a49de347/components/offline_pages/core/model/create_archive_task.cc
[delete] https://crrev.com/ab8978bb4b2ee8f7bda17209d340ac76a49de347/components/offline_pages/core/model/create_archive_task.h
[delete] https://crrev.com/ab8978bb4b2ee8f7bda17209d340ac76a49de347/components/offline_pages/core/model/create_archive_task_unittest.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/components/offline_pages/core/model/delete_page_task.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/components/offline_pages/core/model/offline_page_model_taskified.cc
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/components/offline_pages/core/model/offline_page_model_taskified.h
[modify] https://crrev.com/fe107f3f57e73cf91de9300cc8b3b2a1d7c58c5d/components/offline_pages/core/model/offline_page_model_taskified_unittest.cc

Comment 9 by romax@chromium.org, Jan 9 2018

Status: Fixed (was: Assigned)
After the fix landed, there's no regression for this metric, hence closing the issue as fixed. Please reopen if there's a significant regression.
Thanks!

Sign in to add a comment