New issue
Advanced search Search tips

Issue 856347 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Seeing extra per-tile command buffer flushes with OOP-R

Project Member Reported by vmi...@chromium.org, Jun 25 2018

Issue description

Chrome Version: 69.0.3472.0
OS: OS X tested

The attached traces are from zooming in on usatoday.com with and without OOP-R.  In the OOP-R case I'm consistently seeing a call to CommandBufferHelper::Flush inside RasterImplementation::RasterCHROMIUM.
 
trace_usatoday-oopr.json.gz
3.5 MB Download
trace_usatoday-ganesh.json.gz
2.4 MB Download

Comment 1 by vmi...@chromium.org, Jun 27 2018

Here is the call-stack issuing the Flushes:

 gpu::raster::RasterImplementation::RasterCHROMIUM(...)
  gpu::raster::RasterImplementation::UnmapRasterCHROMIUM(long)
   gpu::ScopedTransferBufferPtr::Release()
    gpu::TransferBuffer::FreePendingToken(void*, unsigned int)
     gpu::CommandBufferHelper::Flush()
      gpu::CommandBufferProxyImpl::Flush(int)

sunnyps@ and I looked at this briefly.  We think it's auto flushing due to crossing a transfer buffer threshold.  See TransferBuffer::bytes_since_last_flush_ and https://cs.chromium.org/chromium/src/gpu/command_buffer/client/transfer_buffer.cc?sq=package:chromium&g=0&l=94

The accounting for TransferBuffer::bytes_since_last_flush_ doesn't seem to account for the way OOP-R maps and then shrinks memory.  We map a large size, then shrink it with TransferBuffer::ShrinkLastBlock().

Comment 2 by vmi...@chromium.org, Jun 27 2018

Cc: piman@chromium.org jdarpinian@chromium.org
Owner: jdarpinian@chromium.org
Status: Assigned (was: Available)
I tried to remove this flushing as part of my work on bug 835353 with this change: https://chromium-review.googlesource.com/c/chromium/src/+/1077568

Unfortunately I had to revert because it regressed Canvas 2D performance on MotionMark ( bug 849348 ). I'm trying again with this new change to grow and shrink the transfer buffer and I'm hoping that resolves the performance issue: https://chromium-review.googlesource.com/c/chromium/src/+/1105505

Comment 4 by enne@chromium.org, Jun 27 2018

Thanks! Glad to hear you were already looking at this from another angle. That last patch seems to address the OOP-R flushing entirely.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 3

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

commit 65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783
Author: James Darpinian <jdarpinian@chromium.org>
Date: Tue Jul 03 07:23:17 2018

GPU: Grow and shrink transfer buffer

Automatically grow the transfer buffer when it is full and shrink it
when the full capacity is not being used. Allow choosing larger and
smaller sizes than before.

Before we would only grow it when a single transfer request was larger
than the whole buffer; requests smaller than the whole buffer would just
block if the buffer was full. Also we would not ever shrink the buffer
until someone called Free() manually.

Bug:  850271 , 835353, 828363,  856347 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: If2f2235a1d5297c63663398b37e1e30791347a3e
Reviewed-on: https://chromium-review.googlesource.com/1105505
Commit-Queue: James Darpinian <jdarpinian@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572135}
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/client_test_helper.cc
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/client_test_helper.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/implementation_base.cc
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/implementation_base.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/mock_transfer_buffer.cc
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/mock_transfer_buffer.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/ring_buffer.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/shared_memory_limits.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/transfer_buffer.cc
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/transfer_buffer.h
[modify] https://crrev.com/65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783/gpu/command_buffer/client/transfer_buffer_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 3

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

commit 6df2efa5313f48302a51d510fa9b5983811c2328
Author: Brian Sheedy <bsheedy@chromium.org>
Date: Tue Jul 03 17:42:32 2018

Revert "GPU: Grow and shrink transfer buffer"

This reverts commit 65c38f54b0ae666e8b27bfa5aef6d3ab92d4a783.

Reason for revert: Causing DCHECK failures in VR instrumentation tests

Original change's description:
> GPU: Grow and shrink transfer buffer
> 
> Automatically grow the transfer buffer when it is full and shrink it
> when the full capacity is not being used. Allow choosing larger and
> smaller sizes than before.
> 
> Before we would only grow it when a single transfer request was larger
> than the whole buffer; requests smaller than the whole buffer would just
> block if the buffer was full. Also we would not ever shrink the buffer
> until someone called Free() manually.
> 
> Bug:  850271 , 835353, 828363,  856347 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
> Change-Id: If2f2235a1d5297c63663398b37e1e30791347a3e
> Reviewed-on: https://chromium-review.googlesource.com/1105505
> Commit-Queue: James Darpinian <jdarpinian@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#572135}

TBR=jdarpinian@chromium.org,sunnyps@chromium.org,piman@chromium.org

Change-Id: I031b30234cdcfbd8a4e48c7ebb6fd23312ec10ed
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  850271 , 835353, 828363,  856347 ,  859952 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1124822
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572277}
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/client_test_helper.cc
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/client_test_helper.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/implementation_base.cc
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/implementation_base.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/mock_transfer_buffer.cc
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/mock_transfer_buffer.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/ring_buffer.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/shared_memory_limits.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/transfer_buffer.cc
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/transfer_buffer.h
[modify] https://crrev.com/6df2efa5313f48302a51d510fa9b5983811c2328/gpu/command_buffer/client/transfer_buffer_unittest.cc

Owner: kbr@chromium.org
Taking over from James while he's OOO.

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 13

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

commit 1c3333c5acf70c841a66feba15273b1dc7d76da0
Author: James Darpinian <jdarpinian@chromium.org>
Date: Fri Jul 13 01:56:36 2018

GPU: Grow and shrink transfer buffer

Second try after revert 6df2efa5313f48302a51d510fa9b5983811c2328.
Original at https://crrev.com/c/1105505

Automatically grow the transfer buffer when it is full and shrink it
when the full capacity is not being used. Start at a smaller size.

Before we would only grow it when a single transfer request was larger
than the whole buffer; requests smaller than the whole buffer would just
block if the buffer was full. Also we would not ever shrink the buffer
until someone called Free() manually.

Bug:  850271 , 835353, 828363,  856347 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Id1223267d0ab444afe18f62f45ae5f365131e18c
Reviewed-on: https://chromium-review.googlesource.com/1125503
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: James Darpinian <jdarpinian@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574816}
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/client_test_helper.cc
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/client_test_helper.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/implementation_base.cc
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/implementation_base.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/mock_transfer_buffer.cc
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/mock_transfer_buffer.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/ring_buffer.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/shared_memory_limits.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/transfer_buffer.cc
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/transfer_buffer.h
[modify] https://crrev.com/1c3333c5acf70c841a66feba15273b1dc7d76da0/gpu/command_buffer/client/transfer_buffer_unittest.cc

Status: Fixed (was: Assigned)
Seems to be sticking this time.
Owner: jdarpinian@chromium.org

Sign in to add a comment