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

Issue 911410 link

Starred by 1 user

Issue metadata

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

Blocked on:
issue 911511
issue 911922



Sign in to add a comment

11.9% regression gpu:effective_size_avg, win7 - media.desktop at 605340:610161

Project Member Reported by chcunningham@chromium.org, Dec 4

Issue description

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

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


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

Win 7 Nvidia GPU Perf

media.desktop - Benchmark documentation link:
  None
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/17993d9a140000

All of the runs failed. The most common error (20/20 runs) was:
SwarmingTestError: The test failed. No Python exception was found in the log.
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/103fe86a140000

All of the runs failed. The most common error (20/20 runs) was:
SwarmingTestError: The test failed. No Python exception was found in the log.
Blockedon: 911922
Blockedon: 911511
^^ re-running now that blocking bugs are fixed
Cc: kristip...@chromium.org piman@chromium.org
Owner: kristip...@chromium.org
Status: Assigned (was: Untriaged)
📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/17b8307a140000

Use SharedImageInterface for one-copy staging buffers by piman@chromium.org
https://chromium.googlesource.com/chromium/src/+/9058f7866e74b92eeef1c30780522fba0740d2f3
memory:chrome:all_processes:reported_by_chrome:gpu:effective_size: 1.436e+07 → 1.815e+07 (+3.788e+06)

Disable new thumbnail captures for TopSites by kristipark@chromium.org
https://chromium.googlesource.com/chromium/src/+/488feec7a2065da4c517b2e17c867dec17bc0619
memory:chrome:all_processes:reported_by_chrome:gpu:effective_size: 1.822e+07 → 1.605e+07 (-2.17e+06)

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

Benchmark documentation link:
  None
Cc: backer@chromium.org
Owner: piman@chromium.org
cc'ing backer who reviewed piman's change.
Looking...
Owner: backer@chromium.org
I can't repro this on linux. The obvious bug was fixed (https://chromium-review.googlesource.com/c/chromium/src/+/1347355). Perhaps there's something deeper here and will need to try on windows.
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 13

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

commit cb47bde18f479a47021327abb0a8fec8086ba06a
Author: Jonathan Backer <backer@chromium.org>
Date: Thu Dec 13 23:36:18 2018

FlushPendingWork if we destroyed_buffers

This is a speculative fix for  https://crbug.com/911410 .

With SharedImage, we need to FlushPendingWork to guarantee that
a DestroySharedImage is sent to the service side. This call was
introduced in https://crrev.com/c/chromium/src/+/1347355

Before this new CL, we would only FlushPendingWork if we were able to
clear all free_buffers_, clear all busy_buffers_, and destroyed_buffers
was set.

With this new slight change, we will FlushPendingWork if destroyed_buffers
was set, regardless if there are remaining free_buffers_ and
busy_buffers_.

This change is correct because free_buffers_ is LRU, busy_buffers_ is LRU,
and free_buffers_ are all older than busy_buffers_.

Bug:  911410 
Change-Id: I416cd467139e03d55b46cf8476d539cd8d25d7e1
Reviewed-on: https://chromium-review.googlesource.com/c/1377190
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616489}
[modify] https://crrev.com/cb47bde18f479a47021327abb0a8fec8086ba06a/cc/raster/staging_buffer_pool.cc

I finally hunted down the bug and it's an instrumentation error. There is no memory regression, but the instrumentation on windows was broken. I have a CL up for review.
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 19

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

commit cee8542f5439b2a3d90b26aa9e7910d258a547f6
Author: Jonathan Backer <backer@chromium.org>
Date: Wed Dec 19 22:07:04 2018

Fix up GMB accounting with passthrough

When we switched over OneCopyRasterBufferProvider to use SharedImage for
GMB staging buffers, we broke the memory instrumentation for
passthrough decoding.

StagingBuffer will create a MemoryDump owning the SHM associated with
the GMB. This CL adds a link from the SharedImage to the GMB on the
service side for passthrough. Given the low priority of the GLImage link
to SHM, the effective size of of the SharedImage in the GPU process
drops to zero because the client has a higher priority link to the SHM.

Bug:  911410 
Change-Id: Ia1161222383c55ce020a60065df0f4a4d9eaa13d
Reviewed-on: https://chromium-review.googlesource.com/c/1384644
Reviewed-by: Eric Karl <ericrk@chromium.org>
Commit-Queue: Jonathan Backer <backer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617972}
[modify] https://crrev.com/cee8542f5439b2a3d90b26aa9e7910d258a547f6/gpu/command_buffer/service/shared_image_backing_factory_gl_texture.cc

Status: Fixed (was: Assigned)
Woot. Eliminating the double counting with passthrough fixed the graph. I love how fast this graph updated. Sometimes I have to wait a week.
Cc: bpastene@chromium.org chromium...@skia-public.iam.gserviceaccount.com oysteine@chromium.org beccahughes@chromium.org kdillon@chromium.org etiennep@chromium.org rockot@google.com
 Issue 911412  has been merged into this issue.

Sign in to add a comment