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

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


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

linux-perf

media.desktop - Benchmark documentation link:
  None

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/11abfbe4e40000

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/10ed7c04e40000

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/11abfbe4e40000

Use SharedImage for HUD layer and one copy raster by backer@chromium.org
https://chromium.googlesource.com/chromium/src/+/e29d023ec416e0931584cb4c6daaa478cd36b478
memory:chrome:all_processes:reported_by_chrome:gpu:effective_size: 1.247e+07 → 1.256e+07 (+9.293e+04)

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

Benchmark documentation link:
  None

📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/10ed7c04e40000

Use SharedImage for HUD layer and one copy raster by backer@chromium.org
https://chromium.googlesource.com/chromium/src/+/e29d023ec416e0931584cb4c6daaa478cd36b478
memory:chrome:all_processes:reported_by_chrome:gpu:effective_size: 1.248e+07 → 1.257e+07 (+9.54e+04)

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

Benchmark documentation link:
  None

I did some investigation and here is what is happening.

OneCopyRasterBufferProvider::RasterBufferImpl and OneCopyGpuBacking::GpuBacking are pretty tightly coupled. IIUC, RasterBufferImpl is mostly accessed from worker threads and GpuBacking is mostly accessed from compositor thread. RasterBufferImpl is meant to be short lived and GpuBacking is long lived.

SharedImage (GPU memory allocation) is created on worker thread in RasterBufferImpl::Playback. The resulting mailbox for the allocation is not copied over to GpuBacking until ~RasterBufferImpl.  Until the mailbox is copied over, GpuBacking::OnMemoryDump will early exit and memory will not be attributed to the client.

We have a benign race where RasterBufferImpl::Playback on worker thread has occurred, but ~RasterBufferImpl has not yet been called on compositor worker thread. This is benign because ~RasterBufferImpl will eventually be called. But it screws up our memory dumping accounting.

The same race probably occurs with GpuRasterBufferProvider.

+cc piman

This particular benign race is present in GpuRasterBufferProvider as well and was introduced with 6283c799ca8e416c3ee6fb7d195191601bb5663b (https://chromium-review.googlesource.com/c/chromium/src/+/1242080)

We could try to fix up the memory accounting to make the test less flaky, but I don't know if it is worth it.

WDYT Antoine?

Just to be clear, the race is that we do the memory dump after OCRBP::RasterBufferImpl::Playback (on worker thread), but before OCRBP::~RasterBufferImpl which is triggered on compositor thread (by TileManagerImpl::CheckForCompletedTasks). This prevents the client from claiming the memory in OCRBP::OnCopyGpuBacking::OnMemoryDump.

To fix it, we could have the RasterBufferImpls also dump memory, and make sure somehow their dump gets aliased with the GpuBacking's. But then we'd need to protect the mailbox and whatever else with a lock because we'd need to dump the RasterBufferImpls on the compositor thread I think (because worker threads don't have thread runners).

We could also go back to having the mailbox generated by the client, so that we can store the mailbox in the GpuBacking and get the dumps to alias properly before the ~RasterBufferImpl, but it makes the API more error prone.

Either one seems unfortunate, and since this is not an actual regression, and the accounting mismatch is bounded by the tasks that are in-flight at the time we dump, I'd suggest punting, unless we find a better way.

Punting as per comment #11.