1003450 - Dependent priorities for http/2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

Since http/2 has a way better priority scheme than http/1 (ok, not saying much there), we should make it possible for us to make use of that beyond weighting resources according to their gecko priority.

Specifically, we want to at least be able to mark streams for resources on a page (images, stylesheets, js) as dependent on the page itself, for easy reprioritization based on the currently-viewable tab.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: [1/2] - node-http2 refuses PRIORITY in idle state

This only impacts the CI

node-http2 as of at least 3.0.1 generates protocol error upon receipt
of PRIORITY frames in the IDLE state.. the http2 makes this clearly
legal in 5.1 Stream States -> idle  "Receiving any frames other than
HEADERS, PUSH_PROMISE or PRIORITY on a stream in this state MUST be treated as a connection error
(Section 5.4.1) of type PROTOCOL_ERROR."

Fix sent upstream as well as https://github.com/molnarg/node-http2/pull/90

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: [2/2] Group Dependency nodes for HTTP/2

Comment 3

[u408661]

Comment on attachment 8534403 [details] [diff] [review]
[1/2] - node-http2 refuses PRIORITY in idle state

Review of attachment 8534403 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine for our purposes (since we don't run node-http2 tests), but the upstream fix needs some... fixing :)

We'll also want to update our upstream copy eventually, though likely not until upstream goes full h2 instead of h2-draft (we've already diverged on version numbers for our testing purposes), but that's for another bug.

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bz://1003450/mcmanus

Comment 5

[Patrick McManus [:mcmanus]]

/r/1395 - bug 1003450 [1/2] - node-http2 refuses PRIORITY in idle state r=hurley
/r/1397 - bug 1003450 - [2/2] Group Dependency nodes for HTTP/2 r=hurley

Pull down these commits:

hg pull review -r 7b57b159bc6d70f1f7fe72b70d42182743c9b707

Comment 6

[u408661]

https://reviewboard.mozilla.org/r/1395/#review821

Ship It!

Comment 7

[u408661]

https://reviewboard.mozilla.org/r/1397/#review825

This looks pretty good to me so far, but there's a couple notes below. Most important is the flexibility of nsIClassOfService.

Beyond that (and the discussed-on-irc part 3 to make this conditional on h2-16), have you tested this against any live servers to see how badly these never-opened priority pseudo-streams breaks them? :)

::: netwerk/base/public/nsIClassOfService.idl
(Diff revision 1)
> +  const unsigned long Blocker = 1 << 0;

Some comments about what these actually mean (ASCII art of the diagram from #mozlandia optional) would go a long way to understanding by future readers.

::: netwerk/protocol/http/Http2Session.h
(Diff revision 1)
> +    kBlockerGroupID =     0x3,
> +    kOtherGroupID =       0x5,
> +    kBackgroundGroupID =  0x7,
> +    kSpeculativeGroupID = 0x9,
> +    kFollowerGroupID =    0xB

So there's a disconnect between "blocker" and "follower" (at least in my mind). Better (I think) would either be to use "leader" and "follower" like in Portland, or "blocker" and "blocked" (or "blockee"?).

This of course applies to other uses of blocker and follower in the code.

::: netwerk/protocol/http/Http2Stream.cpp
(Diff revision 1)
> +  // explicit bg streams (not yet used) depend on 7

Either the code for SendBeacon is wrong, or this comment is, because in SendBeacon you explicitly use background :)

::: netwerk/protocol/http/Http2Stream.cpp
(Diff revision 1)
> +    mPriorityDependency = Http2Session::kFollowerGroupID; // unmarked followers

I'm really hoping we got all the explicit markers right. It would be a shame to make a stream block that shouldn't/doesn't need to

::: netwerk/protocol/http/HttpChannelChild.cpp
(Diff revision 1)
> +HttpChannelChild::AddClassFlags(uint32_t inFlags) 

nit: trailing whitespace

::: netwerk/protocol/http/nsHttpChannel.cpp
(Diff revision 1)
>      // Don't consider mLoadUnblocked here, since it's not indication of a demand
>      // to load prioritly. It's mostly used to load XHR requests, but those should

Don't reference mLoadUnblocked, since it's gone. Also, while you're here, s/prioritly/<something else - I'm not quite sure what word belongs there - priority maybe?>/

::: netwerk/base/public/nsIClassOfService.idl
(Diff revision 1)
> +  void clearClassFlags(in unsigned long flags);

Do you really envision having multiple classes/class flags set for a channel? This seems overly flexible for the current use case.

If there is a future state where we're definitely going to be using multiple flags/channel, then fine, but there needs to be some sanity checking when setting the flags (it is, for example, rather nonsensical to have a channel labeled as both a blocker and a follower).

::: netwerk/protocol/http/HttpChannelChild.cpp
(Diff revision 1)
> +HttpChannelChild::ClearClassFlags(uint32_t inFlags) 

nit: trailing whitespace

Comment 8

[Patrick McManus [:mcmanus]]

Thanks nick

(In reply to Nicholas Hurley [:hurley] from comment #7)

> h2-16), have you tested this against any live servers to see how badly these
> never-opened priority pseudo-streams breaks them? :)

twitter and google seem fine with it - thankfully.

> > +    mPriorityDependency = Http2Session::kFollowerGroupID; // unmarked followers
> 
> I'm really hoping we got all the explicit markers right. It would be a shame
> to make a stream block that shouldn't/doesn't need to

that's just backwards compatible with the h1 path and the blocker/unblocked semantics I removed with this patch.. things that weren't blocker/unblocked meant follower. So it isn't breaking new ground.

> ::: netwerk/base/public/nsIClassOfService.idl
> (Diff revision 1)
> > +  void clearClassFlags(in unsigned long flags);
> 
> Do you really envision having multiple classes/class flags set for a
> channel? This seems overly flexible for the current use case.

I think its a real possibility - perhaps a bit for the image geometry case, and plausibly things like latency-sensitive.

> If there is a future state where we're definitely going to be using multiple
> flags/channel, then fine, but there needs to be some sanity checking when
> setting the flags (it is, for example, rather nonsensical to have a channel
> labeled as both a blocker and a follower).

I think I prefer to just resolve it at use-time through precedence.. that way a ((follower | leader) &= ~follower) == leader.

Comment 9

[Patrick McManus [:mcmanus]]

/r/1395 - bug 1003450 [1/3] - node-http2 refuses PRIORITY in idle state r=hurley
/r/1397 - bug 1003450 - [2/3] Group Dependency nodes for HTTP/2 r=hurley
/r/1417 - bug 1003450 - [3/3] Group Dependency Nodes require >= h2-16 r=hurley

Pull down these commits:

hg pull review -r 6827fc2c10edb398c977f826fe43c8762473b3bf

Comment 10

[u408661]

https://reviewboard.mozilla.org/r/1417/#review851

::: netwerk/protocol/http/Http2Session.cpp
(Diff revision 1)
> -  if (gHttpHandler->UseH2Deps() && gHttpHandler->CriticalRequestPrioritization()) {
> +  fprintf(stderr,"version is %d -15 is %d\n", mVersion, HTTP_VERSION_2_DRAFT_15);

Get rid of this fprintf

::: netwerk/protocol/http/Http2Session.cpp
(Diff revision 1)
> +  // allow our priority scheme. Blacklist them here - there are aliased

s/there/they/

Comment 11

[u408661]

https://reviewboard.mozilla.org/r/1393/#review913

Alright... I'm still a little iffy on the whole multiple-flag thing (specifically the ability to have both leader & follower set at the same time), but I'm confident enough in convention and reviewing that the chances of hitting a silly-overlap case is probably pretty slim.

Comment 12

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=a2bb7c46e338

and I reverified that it works ok today with twitter h2-15. gfe took down h2 support last night so I can't retest that, but it worked fine a week ago.

Comment 13

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92c282366849
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62bf7841725
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d5c1ad6e4f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92c282366849
https://hg.mozilla.org/mozilla-central/rev/f62bf7841725
https://hg.mozilla.org/mozilla-central/rev/a6d5c1ad6e4f

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1003450 - [3/3] Group Dependency Nodes require >= h2-16 r=hurley

Comment 17

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1003450 [1/3] - node-http2 refuses PRIORITY in idle state r=hurley

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: MozReview Request: bug 1003450 - [2/3] Group Dependency nodes for HTTP/2 r=hurley