Rename and modify SpdyPriorityTree.

Rename and modify SpdyPriorityTree.

Rename SpdyPriorityTree to Http2PriorityWriteScheduler and move into
http2_write_scheduler.h, to better align with PriorityWriteScheduler, which
handles the equivalent role for SPDY/3. Also change code and comments to refer
to streams instead of nodes.

Modify Http2PriorityWriteScheduler API to be more consistent with
PriorityWriteScheduler. Previously, in cases of API misuse,
Http2PriorityWriteScheduler methods would return false to indicate error, in
turn sometimes requiring other values to be returned via out-params. This CL
makes Http2PriorityWriteScheduler use a style similar to PriorityWriteScheduler,
where invalid method calls trigger DFATAL, and status is no longer reflected by
return value.

Modify Http2PriorityWriteScheduler so that it picks streams one at a time, as
opposed to calculating a full ordering across all streams in a batch. Picking
streams one at a time is necessary to match usage of existing
WriteBlockedList/PriorityWriteScheduler by SPDY and QUIC code. This change also
adds proper round-robining of picks across usable streams with the same
priority.

This CL lands server changes 112594726, 112597015, and 112782384 by mpw.

BUG=488484
Committed: https://crrev.com/09c6fbd4281c0c4a5f22093f8791dddc01a50561
Cr-Commit-Position: refs/heads/master@{#373743}

[Bence, 2016-02-03 18:27:44]

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

[Bence, 2016-02-03 18:27:45]

Ryan: PTAL.  Originally I am using LinkedList and LinkNode from
base/containers/linked_list.h, which is similar to the container used server
side in terms of features, but a bit different in semantics (most notably,
LinkNode<T> cannot be cast to T, one needs to call its value() method, also it
does not have begin(), and insertion methods are named differently).  This would
cause maintenance burden, because this is an infrequently used class.  Since the
implementation has to be different from server side anyway, another option is to
use std::list, which I do in Patch Set.  This is more familiar to developers,
though the std::find in line 631 is O(n) instead of O(1).

Do you prefer me to use base::LinkedList or std::list here?

Thanks.

[Ryan Hamilton, 2016-02-04 01:25:39]

lgtm

[Ryan Hamilton, 2016-02-04 01:28:01]

On 2016/02/03 18:27:45, Bence wrote:
> Ryan: PTAL.  Originally I am using LinkedList and LinkNode from
> base/containers/linked_list.h, which is similar to the container used server
> side in terms of features, but a bit different in semantics (most notably,
> LinkNode<T> cannot be cast to T, one needs to call its value() method, also it
> does not have begin(), and insertion methods are named differently).  This
would
> cause maintenance burden, because this is an infrequently used class.  Since
the
> implementation has to be different from server side anyway, another option is
to
> use std::list, which I do in Patch Set.  This is more familiar to developers,
> though the std::find in line 631 is O(n) instead of O(1).
> 
> Do you prefer me to use base::LinkedList or std::list here?
> 
> Thanks.

I think either approach is fine. Perhaps LinkedList is nice because of the
performance?

Also, it's too bad that we lose version history completely here :( Is there any
way to trick git into noticing that this is a rename?

[Bence, 2016-02-04 14:09:56]

On 2016/02/04 01:28:01, Ryan Hamilton wrote:
> 
> I think either approach is fine. Perhaps LinkedList is nice because of the
> performance?
> 

Done.


> 
> Also, it's too bad that we lose version history completely here :( Is there
any
> way to trick git into noticing that this is a rename?
> 

Yes, there is.  Let's land https://crrev.com/1668773003 first, then I'll rebase
this one on that.

[Ryan Hamilton, 2016-02-04 20:17:50]

On 2016/02/04 14:09:56, Bence wrote:
> On 2016/02/04 01:28:01, Ryan Hamilton wrote:
> > 
> > I think either approach is fine. Perhaps LinkedList is nice because of the
> > performance?
> > 
> 
> Done.
> 
> 
> > 
> > Also, it's too bad that we lose version history completely here :( Is there
> any
> > way to trick git into noticing that this is a rename?
> > 
> 
> Yes, there is.  Let's land https://crrev.com/1668773003 first, then I'll
rebase
> this one on that.

Awesome, thanks!

[Bence, 2016-02-05 02:17:55]

The CQ bit was checked by bnc@chromium.org

[Bence, 2016-02-05 02:17:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org
Link to the patchset: https://codereview.chromium.org/1660283002/#ps120001
(title: "Rebase on https://crrev.com/1668773003.")

[commit-bot: I haz the power, 2016-02-05 02:18:10]

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

[commit-bot: I haz the power, 2016-02-05 04:13:29]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-05 04:15:06]

Description was changed from

==========
Rename and modify SpdyPriorityTree.

Rename SpdyPriorityTree to Http2PriorityWriteScheduler and move into
http2_write_scheduler.h, to better align with PriorityWriteScheduler, which
handles the equivalent role for SPDY/3. Also change code and comments to refer
to streams instead of nodes.

Modify Http2PriorityWriteScheduler API to be more consistent with
PriorityWriteScheduler. Previously, in cases of API misuse,
Http2PriorityWriteScheduler methods would return false to indicate error, in
turn sometimes requiring other values to be returned via out-params. This CL
makes Http2PriorityWriteScheduler use a style similar to PriorityWriteScheduler,
where invalid method calls trigger DFATAL, and status is no longer reflected by
return value.

Modify Http2PriorityWriteScheduler so that it picks streams one at a time, as
opposed to calculating a full ordering across all streams in a batch. Picking
streams one at a time is necessary to match usage of existing
WriteBlockedList/PriorityWriteScheduler by SPDY and QUIC code. This change also
adds proper round-robining of picks across usable streams with the same
priority.

This CL lands server changes 112594726, 112597015, and 112782384 by mpw.

BUG=488484
==========

to

==========
Rename and modify SpdyPriorityTree.

Rename SpdyPriorityTree to Http2PriorityWriteScheduler and move into
http2_write_scheduler.h, to better align with PriorityWriteScheduler, which
handles the equivalent role for SPDY/3. Also change code and comments to refer
to streams instead of nodes.

Modify Http2PriorityWriteScheduler API to be more consistent with
PriorityWriteScheduler. Previously, in cases of API misuse,
Http2PriorityWriteScheduler methods would return false to indicate error, in
turn sometimes requiring other values to be returned via out-params. This CL
makes Http2PriorityWriteScheduler use a style similar to PriorityWriteScheduler,
where invalid method calls trigger DFATAL, and status is no longer reflected by
return value.

Modify Http2PriorityWriteScheduler so that it picks streams one at a time, as
opposed to calculating a full ordering across all streams in a batch. Picking
streams one at a time is necessary to match usage of existing
WriteBlockedList/PriorityWriteScheduler by SPDY and QUIC code. This change also
adds proper round-robining of picks across usable streams with the same
priority.

This CL lands server changes 112594726, 112597015, and 112782384 by mpw.

BUG=488484

Committed: https://crrev.com/09c6fbd4281c0c4a5f22093f8791dddc01a50561
Cr-Commit-Position: refs/heads/master@{#373743}
==========

[commit-bot: I haz the power, 2016-02-05 04:15:07]

Patchset 7 (id:??) landed as
https://crrev.com/09c6fbd4281c0c4a5f22093f8791dddc01a50561
Cr-Commit-Position: refs/heads/master@{#373743}