New issue
Advanced search Search tips

Issue 850085 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: 1
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

PrioritizedTaskRunner needs to tie-break same priority requests with posted order

Project Member Reported by jkarlin@chromium.org, Jun 6 2018

Issue description

If two tasks have the same priority, they should run in posting order. This can prevent hypothetical task starvation. It's also necessary for the SimpleCachePrioritization control group to ensure that we maintain old behavior.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 6 2018

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

commit 413675e9b667e07ae84a115268abedfdb7430740
Author: Josh Karlin <jkarlin@chromium.org>
Date: Wed Jun 06 17:02:59 2018

Run PrioritizedTaskRunner tasks with the same priority in posting order

What:
If two tasks have the same priority, they should run in posting order.

A second thing this CL does is fix a race in the unittests. I meant to block
the task runner before queueing up requests but failed to. Fixed here.

Why:
This can prevent hypothetical task starvation. It's also necessary for the
SimpleCachePrioritization control group to ensure that we maintain old
behavior.

Bug:  850085 
Change-Id: I1f259296604bf04ea33ecbee9ad3180f064c4266
Reviewed-on: https://chromium-review.googlesource.com/1088759
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564933}
[modify] https://crrev.com/413675e9b667e07ae84a115268abedfdb7430740/net/base/prioritized_task_runner.cc
[modify] https://crrev.com/413675e9b667e07ae84a115268abedfdb7430740/net/base/prioritized_task_runner.h
[modify] https://crrev.com/413675e9b667e07ae84a115268abedfdb7430740/net/base/prioritized_task_runner_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 6 2018

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

commit cf61ddd65c0c26adfa89819fc1df16244dd7c8c4
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Wed Jun 06 23:35:05 2018

Revert "Run PrioritizedTaskRunner tasks with the same priority in posting order"

This reverts commit 413675e9b667e07ae84a115268abedfdb7430740.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 564933 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzQxMzY3NWU5YjY2N2UwN2FlODRhMTE1MjY4YWJlZGZkYjc0MzA3NDAM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20ChromiumOS%20MSan%20Tests/7480

Sample Failed Step: browser_tests

Original change's description:
> Run PrioritizedTaskRunner tasks with the same priority in posting order
> 
> What:
> If two tasks have the same priority, they should run in posting order.
> 
> A second thing this CL does is fix a race in the unittests. I meant to block
> the task runner before queueing up requests but failed to. Fixed here.
> 
> Why:
> This can prevent hypothetical task starvation. It's also necessary for the
> SimpleCachePrioritization control group to ensure that we maintain old
> behavior.
> 
> Bug:  850085 
> Change-Id: I1f259296604bf04ea33ecbee9ad3180f064c4266
> Reviewed-on: https://chromium-review.googlesource.com/1088759
> Commit-Queue: Josh Karlin <jkarlin@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564933}

Change-Id: Id4930090c0026aefa9d92bb8871e809a3fcfa124
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  850085 
Reviewed-on: https://chromium-review.googlesource.com/1089892
Cr-Commit-Position: refs/heads/master@{#565096}
[modify] https://crrev.com/cf61ddd65c0c26adfa89819fc1df16244dd7c8c4/net/base/prioritized_task_runner.cc
[modify] https://crrev.com/cf61ddd65c0c26adfa89819fc1df16244dd7c8c4/net/base/prioritized_task_runner.h
[modify] https://crrev.com/cf61ddd65c0c26adfa89819fc1df16244dd7c8c4/net/base/prioritized_task_runner_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 7 2018

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

commit ae585b6551b8dd2f75da7203727b62140d20162d
Author: Josh Karlin <jkarlin@chromium.org>
Date: Thu Jun 07 14:27:16 2018

Reland of "Run PrioritizedTaskRunner tasks with the same priority in posting order"

Reland of: https://chromium-review.googlesource.com/c/chromium/src/+/1088759

I forgot to initialize a member variable.

Bug:  850085 
Change-Id: Idae7c9fe8f658f5d6657881354d6254c302c53e6
Reviewed-on: https://chromium-review.googlesource.com/1090811
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Commit-Queue: Josh Karlin <jkarlin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565260}
[modify] https://crrev.com/ae585b6551b8dd2f75da7203727b62140d20162d/net/base/prioritized_task_runner.cc
[modify] https://crrev.com/ae585b6551b8dd2f75da7203727b62140d20162d/net/base/prioritized_task_runner.h
[modify] https://crrev.com/ae585b6551b8dd2f75da7203727b62140d20162d/net/base/prioritized_task_runner_unittest.cc

Sign in to add a comment