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

Issue 781585 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 783157

Blocking:
issue 788766
issue 786820



Sign in to add a comment

RenderWorker context becomes High priority in SMOOTHNESS_TAKES_PRIORITY mode

Project Member Reported by vmi...@chromium.org, Nov 5 2017

Issue description

Chrome Version: 64.0.3256.0
OS: OSX 10.12.6

When doing a smooth zoom (double-tap-to-zoom) on cnn.com homepage, I am sometimes seeing jank that could possibly be avoided with GPU service scheduling.

See for example attached trace between 2,405ms and 2,436ms.  Renderer tree priority is SMOOTHNESS_TAKES_PRIORITY, however in the GPU scheduler, RenderWorker is getting "High" priority.

We should understand what can cause this.
 
trace_cnn_zoom_highprio_in_smoothness_mode.json.gz
4.1 MB Download
Cc: sunn...@chromium.org
Owner: vmi...@chromium.org
Status: Started (was: Assigned)
This issue is we currently bump streams to High priority if it has _any_ waiting
streams.  In this case, the render compositor (Normal priority) is waiting on the
render worker (Low priority).

I have a patch that addresses this by propagating the waiting stream's priority.

  https://chromium-review.googlesource.com/#/c/chromium/src/+/754415
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 9 2017

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

commit 56eadad0e85ad5331dea863690efc604e3d763b4
Author: Victor Miura <vmiura@chromium.org>
Date: Thu Nov 09 04:30:37 2017

gpu scheduler: Compute stream priorities based on priority of waiters.

Previously, a stream tracked all of it's fences that had waiting streams
in 'release_fences'.  If 'release_fences' was non-empty, the stream's
priority got bumped up to High.  This means a stream with Normal priority
waiting on Low priority stream could bump that stream up to High priority.

This change removes that tracking, and instead keeps count of the number
of waiting streams at each StreamPriority.  The highest priority with non-
zero count becomes the stream's priority.

When a stream changes priority, it recursively propagates it's new priority
to all streams it's waiting on.

BUG= 781585 

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I1fc96f6ca065a8e1f83ddaa61ae4f9725c324bcb
Reviewed-on: https://chromium-review.googlesource.com/754415
Commit-Queue: Victor Miura <vmiura@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515097}
[modify] https://crrev.com/56eadad0e85ad5331dea863690efc604e3d763b4/gpu/command_buffer/service/scheduler.cc
[modify] https://crrev.com/56eadad0e85ad5331dea863690efc604e3d763b4/gpu/command_buffer/service/scheduler_unittest.cc

Blockedon: 783157
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 9 2017

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

commit 4a3cd6261a6396e364791c5dd9b043f755f84785
Author: Jamie Madill <jmadill@chromium.org>
Date: Thu Nov 09 14:10:13 2017

Revert "gpu scheduler: Compute stream priorities based on priority of waiters."

This reverts commit 56eadad0e85ad5331dea863690efc604e3d763b4.

Reason for revert: Seems to be breaking the Windows WebGL tests flakily. Possible race condition.

[1840:4036:1109/013109.655:FATAL:scheduler.cc(366)] Check failed: waiting_priority_counts_[static_cast<int>(priority)] > 0. 
Backtrace:
	base::debug::StackTrace::StackTrace [0x66413A20+32]
	base::debug::StackTrace::StackTrace [0x6640AB7D+13]
	logging::LogMessage::~LogMessage [0x6638FCEE+78]
	gpu::Scheduler::Sequence::RemoveWaitingPriority [0x67F6616F+119]
	gpu::Scheduler::Sequence::RemoveWaitFence [0x67F67149+85]
	gpu::Scheduler::SyncTokenFenceReleased [0x67F67E4E+58]
	base::internal::Invoker<base::internal::BindState<void (__thiscall gpu::Scheduler::*)(gpu::SyncToken const &,unsigned int,gpu::IdType<gpu::SyncPointOrderData,unsigned int,0>,gpu::IdType<gpu::SyncPointOrderData,unsigned int,0>),base::WeakPtr<gpu::Scheduler [0x67F69773+59]
	gpu::SyncPointClientState::ReleaseFenceSyncHelper [0x67F1EA8F+263]
	gpu::SyncPointClientState::ReleaseFenceSync [0x67F1F21E+112]
	gpu::GpuCommandBufferStub::OnFenceSyncRelease [0x680524E7+125]
	gpu::gles2::GLES2DecoderPassthroughImpl::DoInsertFenceSyncCHROMIUM [0x67FBAE0D+17]
	gpu::gles2::GLES2DecoderPassthroughImpl::HandleInsertFenceSyncCHROMIUM [0x67FAEC42+32]
	gpu::gles2::GLES2DecoderPassthroughImpl::DoCommandsImpl<0> [0x67F7C540+192]
	gpu::gles2::GLES2DecoderPassthroughImpl::DoCommands [0x67F7C16F+33]
<snip>

Example build:
https://ci.chromium.org/buildbot/chromium.gpu.fyi/Win7%20Release%20%28AMD%29/5493

Also affects other Windows configurations.

BUG= 781585 , 783157 

Original change's description:
> gpu scheduler: Compute stream priorities based on priority of waiters.
> 
> Previously, a stream tracked all of it's fences that had waiting streams
> in 'release_fences'.  If 'release_fences' was non-empty, the stream's
> priority got bumped up to High.  This means a stream with Normal priority
> waiting on Low priority stream could bump that stream up to High priority.
> 
> This change removes that tracking, and instead keeps count of the number
> of waiting streams at each StreamPriority.  The highest priority with non-
> zero count becomes the stream's priority.
> 
> When a stream changes priority, it recursively propagates it's new priority
> to all streams it's waiting on.
> 
> BUG= 781585 
> 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: I1fc96f6ca065a8e1f83ddaa61ae4f9725c324bcb
> Reviewed-on: https://chromium-review.googlesource.com/754415
> Commit-Queue: Victor Miura <vmiura@chromium.org>
> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#515097}

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

Change-Id: I398a59a1c039030f0ba0b915d6cb1a1dc582158a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  781585 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/760456
Reviewed-by: Jamie Madill <jmadill@chromium.org>
Commit-Queue: Jamie Madill <jmadill@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515164}
[modify] https://crrev.com/4a3cd6261a6396e364791c5dd9b043f755f84785/gpu/command_buffer/service/scheduler.cc
[modify] https://crrev.com/4a3cd6261a6396e364791c5dd9b043f755f84785/gpu/command_buffer/service/scheduler_unittest.cc

Comment 5 by vmi...@chromium.org, Nov 11 2017

Cc: piman@chromium.org
Making a note here of what the reason for the failure in #4.

Scheduler::Sequence::RemoveWaitFence removed all fences with the same sync_token and order number.  In other words if one command buffer waits on the same sync_token moltiple times, they would all get cleared here.  In theory, all of the associated waits should be cleared together as a sequence of calls to Scheduler::Sequence::RemoveWaitFence, matching the count to Scheduler::Sequence::AddWaitFence.

The bug is that, on the IO thread, we can have an incoming AddWaitFence which changes the priority of the releasing sequence in the middle of a RemoveWaitFence sequence.

#1 (IO Thread)   Stream 1 (Pri-3): AddWaitFence Pri-3 for fence A on Stream 2.
#2 (IO Thread)   Stream 1 (Pri-3): AddWaitFence Pri-3 for fence A on Stream 2.
#3 (Main Thread) Stream 1 (Pri-3): RemoveWaitFence Pri-3 for fence A on Stream 2.
#4 (IO Thread)   Stream 3 (Pri-1): AddWaitFence Pri-1 for fence B on Stream 1.  Bumps Stream 1 priority.  Doesn't propagate to Stream 2 because #3 broke the link.
#5 (Main Thread) Stream 1 (Pri-1): RemoveWaitFence Pri-1 for fence A on Stream 2. OOPS, unexpected priority.

Project Member

Comment 6 by bugdroid1@chromium.org, Nov 18 2017

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

commit 651b0c341c3c1b813250dd4df4c45b423d760e93
Author: Victor Miura <vmiura@chromium.org>
Date: Sat Nov 18 05:03:43 2017

Reland "gpu scheduler: Compute stream priorities based on priority of waiters."

This is a reland of 56eadad0e85ad5331dea863690efc604e3d763b4
Original change's description:
> gpu scheduler: Compute stream priorities based on priority of waiters.
> 
> Previously, a stream tracked all of it's fences that had waiting streams
> in 'release_fences'.  If 'release_fences' was non-empty, the stream's
> priority got bumped up to High.  This means a stream with Normal priority
> waiting on Low priority stream could bump that stream up to High priority.
> 
> This change removes that tracking, and instead keeps count of the number
> of waiting streams at each StreamPriority.  The highest priority with non-
> zero count becomes the stream's priority.
> 
> When a stream changes priority, it recursively propagates it's new priority
> to all streams it's waiting on.
> 
> BUG= 781585 
> 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: I1fc96f6ca065a8e1f83ddaa61ae4f9725c324bcb
> Reviewed-on: https://chromium-review.googlesource.com/754415
> Commit-Queue: Victor Miura <vmiura@chromium.org>
> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
> Reviewed-by: Antoine Labour <piman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#515097}

Bug:  781585 
Change-Id: I67055fe2887a8ffcc8032af32ab221a3d66bedac
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/762145
Commit-Queue: Victor Miura <vmiura@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517702}
[modify] https://crrev.com/651b0c341c3c1b813250dd4df4c45b423d760e93/gpu/command_buffer/service/scheduler.cc
[modify] https://crrev.com/651b0c341c3c1b813250dd4df4c45b423d760e93/gpu/command_buffer/service/scheduler.h
[modify] https://crrev.com/651b0c341c3c1b813250dd4df4c45b423d760e93/gpu/command_buffer/service/scheduler_unittest.cc

Comment 7 by vmi...@chromium.org, Nov 19 2017

Blocking: 786820

Comment 8 by vmi...@chromium.org, Nov 19 2017

Status: Fixed (was: Started)

Comment 9 by kbr@chromium.org, Dec 2 2017

Blocking: 788766

Sign in to add a comment