QUIC - Whne alternate job decides to delay the main(TCP) job and if the

QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org

Committed: https://crrev.com/e0e7f968e7ee05a89720892696ea5757562392de
Cr-Commit-Position: refs/heads/master@{#371123}

[ramant (doing other things), 2016-01-13 04:31:14]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 04:32:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/1

[ramant (doing other things), 2016-01-13 18:20:24]

Patchset #1 (id:1) has been deleted

[ramant (doing other things), 2016-01-13 18:20:31]

Patchset #1 (id:20001) has been deleted

[ramant (doing other things), 2016-01-13 18:20:38]

Patchset #1 (id:40001) has been deleted

[ramant (doing other things), 2016-01-13 18:20:46]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 18:21:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/60001

[ramant (doing other things), 2016-01-13 18:21:51]

Description was changed from

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.
==========

to

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org
==========

[ramant (doing other things), 2016-01-13 18:21:51]

rtenneti@chromium.org changed reviewers:
+ rch@chromium.org

[ramant (doing other things), 2016-01-13 18:27:49]

Hi Ryan,
  When I was logging how long the TCP job is being delayed, I found out if the
TCP is not started when QUIC starts its 0RTT handshake, then we weren't delaying
the TCP job. I think, it is good to delay the TCP job in this case (when
delay_tcp_race option is enabled).

  Would appreciate if you could take a quick look at this CL and see if this is
the right way to fix it or not.

thanks much
raman

[ramant (doing other things), 2016-01-13 18:34:04]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-13 18:34:23]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[ramant (doing other things), 2016-01-13 18:34:26]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-01-13 18:34:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/80001

[commit-bot: I haz the power, 2016-01-13 18:44:39]

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

[commit-bot: I haz the power, 2016-01-13 18:44:40]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[ramant (doing other things), 2016-01-13 22:15:32]

Patchset #1 (id:80001) has been deleted

[ramant (doing other things), 2016-01-14 00:14:17]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-14 00:15:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/120001

[commit-bot: I haz the power, 2016-01-14 00:47:02]

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

[commit-bot: I haz the power, 2016-01-14 00:47:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)

[Ryan Hamilton, 2016-01-19 23:23:27]

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:246: void
HttpStreamFactoryImpl::Job::ResumeWaitingJobAfterDelay(
Does this resume the current job, or a job which is waiting on this job? I
believe it's this job, right? In which case, I think I'd remove "WaitingJob"
from the method name.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:247: const base::TimeDelta& delay) {
Instead if passing in delay here, an alternative would be to set wait_time_ and
use that exclusively. That would be nice, because it would explain why
wait_time_ is cleared after the task is posted. (Though perhaps that should be
cleared in the WaitComplete() method.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:265: // If the job hasn't started,
delay that job by |wait_time_| when it starts.
"that job" or "this job"?

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:274: ResumeWaitingJobAfterDelay(delay);
Is the only case we need to schedule a future wait the case where we're at
STATE_NONE? What if we've made some progress but not all the way to waiting?

I think I might be inclined to write this as:

if (next_state_ == STATE_WAIT_FOR_JOB_COMPLETE)
    ResumeWaitingJobAfterDelay(delay)

if (next_state_ < STATE_WAIT_FOR_JOB) // or something
    wait_time_ = delay;

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:855: if (blocking_job_ ||
!wait_time_.is_zero())
Alternatively, we could always move to the WAIT_FOR_JOB state, and do this logic
there. I wonder if that might be cleaner. what do you think?

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.h:137: // existing SpdySession. In that
case, the http and npn-spdy jobs will race.
Can you update this comment when you have a chance?

[ramant (doing other things), 2016-01-20 04:28:15]

Patchset #3 (id:140001) has been deleted

[ramant (doing other things), 2016-01-20 04:36:10]

Patchset #3 (id:160001) has been deleted

[ramant (doing other things), 2016-01-20 04:37:26]

The CQ bit was checked by rtenneti@chromium.org to run a CQ dry run

[ramant (doing other things), 2016-01-20 04:37:52]

Thanks very much Ryan for comments. Made all the changes. PTAL.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:246: void
HttpStreamFactoryImpl::Job::ResumeWaitingJobAfterDelay(
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> Does this resume the current job, or a job which is waiting on this job? I
> believe it's this job, right? In which case, I think I'd remove "WaitingJob"
> from the method name.

You are right.

Done.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:247: const base::TimeDelta& delay) {
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> Instead if passing in delay here, an alternative would be to set wait_time_
and
> use that exclusively. That would be nice, because it would explain why
> wait_time_ is cleared after the task is posted. (Though perhaps that should be
> cleared in the WaitComplete() method.

Done.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:265: // If the job hasn't started,
delay that job by |wait_time_| when it starts.
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> "that job" or "this job"?

:-). It is |this| job. Updated the comment.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:274: ResumeWaitingJobAfterDelay(delay);
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> Is the only case we need to schedule a future wait the case where we're at
> STATE_NONE? What if we've made some progress but not all the way to waiting?
> 
> I think I might be inclined to write this as:
> 
> if (next_state_ == STATE_WAIT_FOR_JOB_COMPLETE)
>     ResumeWaitingJobAfterDelay(delay)
> 
> if (next_state_ < STATE_WAIT_FOR_JOB) // or something
>     wait_time_ = delay;

+1 to the above and thanks for the very good point.

Done.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:855: if (blocking_job_ ||
!wait_time_.is_zero())
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> Alternatively, we could always move to the WAIT_FOR_JOB state, and do this
logic
> there. I wonder if that might be cleaner. what do you think?

Done.

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/1586513002/diff/120001/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.h:137: // existing SpdySession. In that
case, the http and npn-spdy jobs will race.
On 2016/01/19 23:23:27, Ryan Hamilton wrote:
> Can you update this comment when you have a chance?

Done.

[commit-bot: I haz the power, 2016-01-20 04:38:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/180001

[commit-bot: I haz the power, 2016-01-20 08:43:27]

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

[commit-bot: I haz the power, 2016-01-20 08:43:28]

Dry run: This issue passed the CQ dry run.

[ramant (doing other things), 2016-01-21 19:22:42]

Hi Ryan,
  Would appreciate if you could take another look at this CL.

thanks very much
raman

[Ryan Hamilton, 2016-01-23 00:33:06]

lgtm

[ramant (doing other things), 2016-01-23 00:33:36]

The CQ bit was checked by rtenneti@chromium.org

[Ryan Hamilton, 2016-01-23 00:34:35]

lgtm

[commit-bot: I haz the power, 2016-01-23 00:36:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1586513002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1586513002/180001

[commit-bot: I haz the power, 2016-01-23 01:53:31]

Description was changed from

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org
==========

to

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org
==========

[commit-bot: I haz the power, 2016-01-23 01:53:31]

Committed patchset #3 (id:180001)

[commit-bot: I haz the power, 2016-01-23 01:54:57]

Description was changed from

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org
==========

to

==========
QUIC - Whne alternate job decides to delay the main(TCP) job and if the
TCP job hasn't started, then delay the job by the given delay when it
starts.

Added logging of the delayed time in the main(TCP) job.

R=rch@chromium.org

Committed: https://crrev.com/e0e7f968e7ee05a89720892696ea5757562392de
Cr-Commit-Position: refs/heads/master@{#371123}
==========

[commit-bot: I haz the power, 2016-01-23 01:54:57]

Patchset 3 (id:??) landed as
https://crrev.com/e0e7f968e7ee05a89720892696ea5757562392de
Cr-Commit-Position: refs/heads/master@{#371123}