DevToolsNetworkController: support "limit throughput" network condition.

DevToolsNetworkController: support "limit throughput" network condition.

BUG=245436
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276596

[vsevik, 2014-06-10 17:24:20]

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:50: if (!length)
redundant

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:77: bool was_throttling =
conditions_ && conditions_->IsThrottling();
Can we put the line below to the beginning of this method? Or even to the
calling function?
if (!conditions && client_id != active_client_id_)
    return;

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:90: if (was_throttling)
I think this calls should be noops in case we weren't previously throttling. Can
we avoid these checks?

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:122: CancelThrottles();
Can we join two Cancel... calls?

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:162: int64_t reminder =
ticks % length;
remainder :)

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:61: if (callback_type ==
START || (callback_type == READ && result > 0)) {
Is it possible that result != net::ERR_IO_PENDING for START ? How could this
happen?

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:66: Throttle(result);
Is it OK that we could callback before returning from the original network
transaction method?

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:117: return
SetupCallback(callback, rv, RESTART_IGNORING_LAST_ERROR);
Is it somehow enforced that several of all these methods that setup callback
could not be called together (next could be called before the previous one
called back)

[vsevik, 2014-06-11 08:10:35]

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:38:
controller_->ThrottleTransaction(this, penalty);
We should make sure we do not callback synchronously in case of zero penalty
here.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:57: int
DevToolsNetworkTransaction::SetupCallback(
DCHECK(callback_type_ == NONE);

if (result == net::ERR_IO_PENDING) {
  callback_type_ = callback_type;
  callback_ = callback;
  return result;
}

if (!controller_->ShouldThrottle(request_))
  return result;

// Comment this
if (callback_type == START && result < 0)
  return result;

// Comment this
if (callback_type == READ && result < 0)
  return result;

// Comment this
if (callback_type == READ && result == 0)
  return result;

if (callback_type != START && callback_type != READ)
  return result;

callback_type_ = callback_type;
callback_ = callback;
// This should never callback synchronously
Throttle(result);
return net::ERR_IO_PENDING;

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:61: if (callback_type ==
START || (callback_type == READ && result > 0)) {
Should we also throttle the result < 0 case here?

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:76: callback_type_ =
NONE;
This is redundant, let's DCHECK(callback_type_ == NONE) in the beginning instead

[vsevik, 2014-06-11 08:39:33]

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:162: int64_t reminder =
ticks % length;
last_throttle_to_get_packet

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:165:
throttles.push_back(throttles_[i]);
std::rotate FTW!

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:222: int64_t best_n;
min_ticks_left

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:224: int64_t n1 =
(throttles_[i].penalty + kPacketSize - 1) / kPacketSize;
penalty_packets_left

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:225: int64_t n = (i + 1)
+ length * (n1 - 1);
ticksLeft

[eustas, 2014-06-11 10:54:26]

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:50: if (!length)
On 2014/06/10 17:24:19, vsevik wrote:
> redundant

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:77: bool was_throttling =
conditions_ && conditions_->IsThrottling();
On 2014/06/10 17:24:19, vsevik wrote:
> Can we put the line below to the beginning of this method? Or even to the
> calling function?
> if (!conditions && client_id != active_client_id_)
>     return;

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:90: if (was_throttling)
On 2014/06/10 17:24:19, vsevik wrote:
> I think this calls should be noops in case we weren't previously throttling.
Can
> we avoid these checks?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:122: CancelThrottles();
On 2014/06/10 17:24:19, vsevik wrote:
> Can we join two Cancel... calls?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:162: int64_t reminder =
ticks % length;
On 2014/06/10 17:24:19, vsevik wrote:
> remainder :)

Thanks =)

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:162: int64_t reminder =
ticks % length;
On 2014/06/11 08:39:32, vsevik wrote:
> last_throttle_to_get_packet

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:165:
throttles.push_back(throttles_[i]);
On 2014/06/11 08:39:32, vsevik wrote:
> std::rotate FTW!

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:222: int64_t best_n;
On 2014/06/11 08:39:32, vsevik wrote:
> min_ticks_left

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:224: int64_t n1 =
(throttles_[i].penalty + kPacketSize - 1) / kPacketSize;
On 2014/06/11 08:39:32, vsevik wrote:
> penalty_packets_left

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:225: int64_t n = (i + 1)
+ length * (n1 - 1);
On 2014/06/11 08:39:32, vsevik wrote:
> ticksLeft

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:38:
controller_->ThrottleTransaction(this, penalty);
On 2014/06/11 08:10:34, vsevik wrote:
> We should make sure we do not callback synchronously in case of zero penalty
> here.

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:57: int
DevToolsNetworkTransaction::SetupCallback(
On 2014/06/11 08:10:34, vsevik wrote:
> DCHECK(callback_type_ == NONE);
> 
> if (result == net::ERR_IO_PENDING) {
>   callback_type_ = callback_type;
>   callback_ = callback;
>   return result;
> }
> 
> if (!controller_->ShouldThrottle(request_))
>   return result;
> 
> // Comment this
> if (callback_type == START && result < 0)
>   return result;
> 
> // Comment this
> if (callback_type == READ && result < 0)
>   return result;
> 
> // Comment this
> if (callback_type == READ && result == 0)
>   return result;
> 
> if (callback_type != START && callback_type != READ)
>   return result;
> 
> callback_type_ = callback_type;
> callback_ = callback;
> // This should never callback synchronously
> Throttle(result);
> return net::ERR_IO_PENDING;

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:61: if (callback_type ==
START || (callback_type == READ && result > 0)) {
On 2014/06/10 17:24:20, vsevik wrote:
> Is it possible that result != net::ERR_IO_PENDING for START ? How could this
> happen?

Yes, SPDY "push" streams can cause OK = 0 result.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:61: if (callback_type ==
START || (callback_type == READ && result > 0)) {
On 2014/06/11 08:10:34, vsevik wrote:
> Should we also throttle the result < 0 case here?

Yes, even ERROR start may have caused network traffic.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:66: Throttle(result);
On 2014/06/10 17:24:20, vsevik wrote:
> Is it OK that we could callback before returning from the original network
> transaction method?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:76: callback_type_ =
NONE;
On 2014/06/11 08:10:34, vsevik wrote:
> This is redundant, let's DCHECK(callback_type_ == NONE) in the beginning
instead

Done.

https://chromiumcodereview.appspot.com/324953002/diff/20001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:117: return
SetupCallback(callback, rv, RESTART_IGNORING_LAST_ERROR);
On 2014/06/10 17:24:20, vsevik wrote:
> Is it somehow enforced that several of all these methods that setup callback
> could not be called together (next could be called before the previous one
> called back)

It is required by transaction contract, but not enforced.
Typical client is a carefully maintained state machine, so it is unlikely that
contract might be broken.

[vsevik, 2014-06-11 12:08:52]

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:23: int64_t penalty;
Looks like we can encapsulate penalty into transaction and get rid of this
structure.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:23: int64_t penalty;
Rename to bytes_left or pending_bytes_count?

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:80:
FireThrottledCallbacks();
We should only do this in case we are continuing to throttle, can we move this
inside "if (conditions_->IsThrottling())" below?
I mean if !conditions->IsThrottling() we already canceled the throttles so it
makes no further sense to fire callbacks.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:147: int64_t shift =
ticks % length;
nit: What is delta and what is shift? Could you please give better  names to
these variables?

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:164:
DevToolsNetworkTransaction* transaction = throttles[i].transaction;
I am pretty sure transaction is never null here, one more reason to encapsulate
penalty in transaction and get rid of the Throttle structure.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:175: Throttles active;
nit: active_transactions

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:176: Throttles finished;
ditto

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:76: // In case of read
error we can't calculate penalty.
What about if (callback_type == START && result < 0) case?

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:84: callback_type_ =
callback_type;
Does it mean we also throttle requests of types other than START and READ now?

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:224:
DCHECK(callback_type_ == READ || callback_type_ == START);
Looks like we can get here from RestartWithAuth now.

[eustas, 2014-06-11 13:18:49]

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:23: int64_t penalty;
On 2014/06/11 12:08:51, vsevik wrote:
> Rename to bytes_left or pending_bytes_count?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:23: int64_t penalty;
On 2014/06/11 12:08:51, vsevik wrote:
> Looks like we can encapsulate penalty into transaction and get rid of this
> structure.

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:80:
FireThrottledCallbacks();
On 2014/06/11 12:08:51, vsevik wrote:
> We should only do this in case we are continuing to throttle, can we move this
> inside "if (conditions_->IsThrottling())" below?
> I mean if !conditions->IsThrottling() we already canceled the throttles so it
> makes no further sense to fire callbacks.

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:164:
DevToolsNetworkTransaction* transaction = throttles[i].transaction;
On 2014/06/11 12:08:51, vsevik wrote:
> I am pretty sure transaction is never null here, one more reason to
encapsulate
> penalty in transaction and get rid of the Throttle structure.

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:175: Throttles active;
On 2014/06/11 12:08:51, vsevik wrote:
> nit: active_transactions

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:176: Throttles finished;
On 2014/06/11 12:08:51, vsevik wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:76: // In case of read
error we can't calculate penalty.
On 2014/06/11 12:08:51, vsevik wrote:
> What about if (callback_type == START && result < 0) case?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:84: callback_type_ =
callback_type;
On 2014/06/11 12:08:52, vsevik wrote:
> Does it mean we also throttle requests of types other than START and READ now?

Done.

https://chromiumcodereview.appspot.com/324953002/diff/60001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:224:
DCHECK(callback_type_ == READ || callback_type_ == START);
On 2014/06/11 12:08:52, vsevik wrote:
> Looks like we can get here from RestartWithAuth now.

Done.

[vsevik, 2014-06-11 14:29:39]

lgtm

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:45: throttles_.erase(
nit: throttled_transactions?

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:71: if (!conditions ||
!conditions->IsThrottling()) {
Shall we drop pending packets in case we went offline?

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
chrome/browser/devtools/devtools_network_controller.cc:159: if (!length)
This could never happen, let's DCHECK instead.

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://chromiumcodereview.appspot.com/324953002/diff/80001/chrome/browser/de...
chrome/browser/devtools/devtools_network_transaction.cc:225:
throttled_byte_count_ -= delta;
nit: inline

[eustas, 2014-06-11 14:42:48]

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_controller.cc (right):

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:45: throttles_.erase(
On 2014/06/11 14:29:39, vsevik wrote:
> nit: throttled_transactions?

Done.

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:71: if (!conditions ||
!conditions->IsThrottling()) {
On 2014/06/11 14:29:39, vsevik wrote:
> Shall we drop pending packets in case we went offline?

Done.

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_controller.cc:159: if (!length)
On 2014/06/11 14:29:39, vsevik wrote:
> This could never happen, let's DCHECK instead.

Done.

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_network_transaction.cc (right):

https://codereview.chromium.org/324953002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_network_transaction.cc:225:
throttled_byte_count_ -= delta;
On 2014/06/11 14:29:39, vsevik wrote:
> nit: inline

Done.

[eustas, 2014-06-11 14:42:55]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-06-11 14:44:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/324953002/80002

[commit-bot: I haz the power, 2014-06-11 21:33:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 21:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:36:51]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[eustas, 2014-06-12 00:50:28]

The CQ bit was checked by eustas@chromium.org

[commit-bot: I haz the power, 2014-06-12 00:51:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eustas@chromium.org/324953002/80002

[commit-bot: I haz the power, 2014-06-12 01:06:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 09:17:02]

Change committed as 276596