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

Issue 778170 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

12% regression in rasterize_and_record_micro.top_25 at 510925:510969

Project Member Reported by kraynov@chromium.org, Oct 25 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Oct 25 2017

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

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


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

android-nexus7v2
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Oct 25 2017

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

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

Hi khushalsagar@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 : Khushal
  Commit : 71ac7cd0994f5688c0677386c2354e79f7a09ca2
  Date   : Mon Oct 23 22:50:05 2017
  Subject: cc: Use Region for tracking positions of images in DiscardableImageMap.

Bisect Details
  Configuration: android_nexus7_perf_bisect
  Benchmark    : rasterize_and_record_micro.top_25
  Metric       : record_time/file___static_top_25_yahooanswers.html
  Change       : 11.96% | 0.377666666667 -> 0.422833333333

Revision             Result                       N
chromium@510924      0.377667 +- 0.00912871       6      good
chromium@510947      0.376 +- 1.35974e-16         6      good
chromium@510948      0.424333 +- 0.00912871       6      bad       <--
chromium@510949      0.424333 +- 0.00912871       6      bad
chromium@510950      0.421 +- 0.0122474           6      bad
chromium@510953      0.424333 +- 0.00912871       6      bad
chromium@510958      0.426167 +- 0.000912871      6      bad
chromium@510969      0.422833 +- 0.0189956        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=file...static.top.25.yahooanswers.html rasterize_and_record_micro.top_25

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

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


For feedback, file a bug with component Speed>Bisection
Cc: enne@chromium.org
Sigh. I'll see if using a StackVector fixes it. Its happening on pages which I double are hitting any case with multiple images, in which case a StackVector of rects should be equivalent to what we had before.
I tested locally on a Android N% with this change: https://chromium-review.googlesource.com/c/chromium/src/+/738677. Here are the record times for image search:

ToT: 2.976 ms
r510948 reverted: 2.064 ms
With above change: 2.153 ms

So there was still a ~0.09 ms regression, but this is expected and necessary to reduce the raster cost later in the pipeline.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 26 2017

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

commit 5efb654c59cb308ce7f008b064ea3a3e17b18035
Author: Khushal <khushalsagar@chromium.org>
Date: Thu Oct 26 20:09:41 2017

cc: Use StackVector of gfx::Rect for discardable image positions.

With [1], we switched to use Region instead of gfx::Rect for tracking
the position of images during discardable image analysis. This causes
a regression in record time for the common case where a single instance
of image exists. Use a StackVector<gfx::Rect, 1> instead to avoid a
heap allocation in these cases.

[1]: https://chromium-review.googlesource.com/c/720245/

R=enne@chromium.org

Bug:  778170 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ifc8646f768c5d04ea0ae32e5dd9a9466448a29d9
Reviewed-on: https://chromium-review.googlesource.com/738677
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511941}
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/base/invalidation_region.cc
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/base/invalidation_region.h
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/layers/picture_layer_impl.cc
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/paint/discardable_image_map.cc
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/paint/discardable_image_map.h
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/paint/discardable_image_map_unittest.cc
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/test/skia_common.cc
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/test/skia_common.h
[modify] https://crrev.com/5efb654c59cb308ce7f008b064ea3a3e17b18035/cc/trees/layer_tree_host_impl_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment