New issue
Advanced search Search tips

Issue 898886 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

10.6%-25.1% regression in rendering.desktop/thread_raster_cpu_time_per_frame at 601705:601843

Project Member Reported by maxlg@chromium.org, Oct 25

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=898886

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


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

Win 7 Perf
mac-10_12_laptop_low_end-perf
mac-10_13_laptop_high_end-perf

rendering.desktop - Benchmark documentation link:
  https://bit.ly/rendering-benchmarks
Cc: khushals...@chromium.org
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/14ead9e9e40000

cc: Ensure bitmap backed images are tracked as discardable. by khushalsagar@chromium.org
https://chromium.googlesource.com/chromium/src/+/bd34e88ade5706c80b60b78ead85af8a251d390a
thread_raster_cpu_time_per_frame: 0.3407 → 0.4227 (+0.082)

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Benchmark documentation link:
  https://bit.ly/rendering-benchmarks
Cc: ericrk@chromium.org
I see what's going on here. Since we treat bitmaps as discardable now, there are 2 extra tasks for decoding and uploading the image which adds to the raster_cpu_time_per_frame. I think these are cases where these is no reuse of the image across tiles because of which this extra task shows up as pure overhead.

This is a trade-off, in the case where the image is shared across tiles, this guarantees that we upload it once and avoids the ref/unref churn for every single use of the image which internally also results in a skia flush for GPU raster. Looking through the overall changes with this patch, there are definitely more improvements than regressions: https://chromeperf.appspot.com/group_report?rev=601717

Still I think we could avoid an extra decode task here, since these are bitmaps so they don't need any decoding. 
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 25

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

commit 388ab98a1a8c12d2d6bf7d7664b6e04100864b41
Author: Khushal <khushalsagar@chromium.org>
Date: Thu Oct 25 23:29:31 2018

cc: Avoid a decode task for bitmaps uploaded using GPU image cache.

These are bitmaps so they don't need a seperate decode task. Avoid the
additional task hop.

R=ericrk@chromium.org

Bug:  898886 
Change-Id: I8b61b99e3f488d8def752b6ea1f3472be3723628
Reviewed-on: https://chromium-review.googlesource.com/c/1299782
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602923}
[modify] https://crrev.com/388ab98a1a8c12d2d6bf7d7664b6e04100864b41/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/388ab98a1a8c12d2d6bf7d7664b6e04100864b41/cc/tiles/gpu_image_decode_cache_unittest.cc

Cc: thakis@chromium.org
 Issue 898875  has been merged into this issue.
Cc: -thakis@chromium.org
motionmark_anim_images_50 has a fps and frame times regression on Nexus 5 which is worth looking at. I see that we are now doing some work on the background tile worker, which I'm assuming are image tasks.

Other than that, the tasks_per_frame regression was reduced by half with the change in #5 as expected.
Status: Fixed (was: Assigned)
Thanks! I observed that the regression has been addressed from the graphs.

Sign in to add a comment