Closed Bug 993037 Opened 6 years ago Closed 6 years ago

http/2 draft 11

Categories

(Core :: Networking: HTTP, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [spdy])

Attachments

(2 files)

OK, so here's a preliminary patch to support draft 11 client-side (I still don't even know if this compiles on all platforms, waiting for that to happen before I release it to the world on the wiki) that interops with nghttp2 just fine.

Right now, dependencies are entirely group-based (extrapolated from the nsISupportsPriority that gecko gives us), since we have no knowledge of resource dependencies at our layer (so far).

I'm still working on the node-http2 support so we can get the unit tests updated.
Attachment #8404352 - Flags: feedback?(mcmanus)
Comment on attachment 8404352 [details] [diff] [review]
part 1 - client implementation

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

until we do image prioritization this makes sense to me
Attachment #8404352 - Flags: feedback?(mcmanus) → feedback+
Attached patch part 2 - testsSplinter Review
And here's the test side of things. I have PRs out on github for Gábor to upstream the node-http2 changes: https://github.com/molnarg/node-http2-protocol/pull/19 and https://github.com/molnarg/node-http2/pull/65
Attachment #8409027 - Flags: review?(mcmanus)
Comment on attachment 8404352 [details] [diff] [review]
part 1 - client implementation

This hasn't changed at all since the feedback round.
Attachment #8404352 - Flags: review?(mcmanus)
Try run: https://tbpl.mozilla.org/?tree=Try&rev=2017e4a36971 (but for reference, the xpcshell tests Work On My Machine(tm)!)
http/2 draft 12 out


http://tools.ietf.org/html/draft-ietf-httpbis-http2-12

would it be better to implement that?
(In reply to ElevenReds from comment #6)
> http/2 draft 12 out

As of late last night, yes. Of course we are going to implement. Doesn't mean we can't get draft11 landed for the time it takes to write and review the draft12 implementation.
(In reply to Nicholas Hurley [:hurley] from comment #7)
> (In reply to ElevenReds from comment #6)
> > http/2 draft 12 out
> 
> As of late last night, yes. Of course we are going to implement. Doesn't
> mean we can't get draft11 landed for the time it takes to write and review
> the draft12 implementation.

Sorry I meant was if it is not ready yet(draft 11)
then you guys can directly switch to draft 12 instead(uplift next week too)
Comment on attachment 8404352 [details] [diff] [review]
part 1 - client implementation

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

nick - this is great.

your code is looking disturbingly just like my code here. I only wish that was unambiguously a compliment :)

let's quickly talk through the setpriority() stuff, and then we can land this sucker.

let's file some followups
 * dependent priorities for managing tab or viewable.
 * alt-svc

::: netwerk/protocol/http/Http2Session.cpp
@@ +1032,5 @@
>    }
>  
>    if (paddingLength > mInputFrameDataSize) {
>      // This is fatal to the session
> +    LOG3(("Http2Session::ParsePadding %p stream 0x%x PROTOCOL_ERROR "

:)

@@ +1057,5 @@
>      self->mExpectedHeaderID = self->mInputFrameID;
>  
> +  if ((self->mInputFrameFlags & kFlag_PRIORITY_GROUP) &&
> +      (self->mInputFrameFlags & kFlag_PRIORITY_DEPENDENCY)) {
> +    RETURN_SESSION_ERROR(self, PROTOCOL_ERROR);

probably a LOG too

@@ +1178,5 @@
>    MOZ_ASSERT(self->mInputFrameType == FRAME_TYPE_PRIORITY);
>  
> +  if ((self->mInputFrameFlags & kFlag_PRIORITY_GROUP) &&
> +      (self->mInputFrameFlags & kFlag_PRIORITY_DEPENDENCY)) {
> +    self->GenerateRstStream(PROTOCOL_ERROR, self->mInputFrameID);

log again

@@ +1183,5 @@
> +    self->ResetDownstreamState();
> +    return NS_OK;
> +  }
> +
> +  if (!(self->mInputFrameFlags & (kFlag_PRIORITY_GROUP | kFlag_PRIORITY_DEPENDENCY))) {

log(priority frame on fire!)

::: netwerk/protocol/http/Http2Stream.cpp
@@ +915,5 @@
>      mSession->TransactionHasDataToWrite(this);  }
>  }
>  
>  void
> +Http2Stream::SetPriority(uint32_t newGroup)

as I understand 5.3.1 (at least for a world without dependencies) N streams can be assigned to a group and the group has a weight - streams don't have a weight independent of the group.

groups compete for a proportionate share of resources .. i.e. group A has weight 1, group B has weight 2 then A gets 1/2 of what B gets. That's true independent of the number of streams in A and B.

streams within the group are equal. So if there are 2 streams in group A and one in B then A1 and A2 each get 1/4 of B1. Does that match your understanding?

We're not doing any grouping in gecko yet, we've just got nice-like priority values.

so that leads me to believe that every stream should be in its own group (i.e. Priority Group Identifier should be stream ID) and that the nice-like-value should be encoded into the weight parameter.

I think with the way you've got this coded the group is derived from the nice-like value.. so 1 stream in a low priority group would end up with too much bandwidth compared to a million streams in a normal priority group.

@@ +918,5 @@
>  void
> +Http2Stream::SetPriority(uint32_t newGroup)
> +{
> +  int32_t httpPriority = static_cast<int32_t>(newGroup);
> +  uint8_t newWeight = (nsISupportsPriority::PRIORITY_LOWEST + 1) -

some definite bounds checking is required around this :)
Attachment #8409027 - Flags: review?(mcmanus) → review+
Attachment #8404352 - Flags: review?(mcmanus)
Comment on attachment 8404352 [details] [diff] [review]
part 1 - client implementation

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

r=mcmanus dependent on landing with draft 12 to get the right priority bits.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +915,5 @@
>      mSession->TransactionHasDataToWrite(this);  }
>  }
>  
>  void
> +Http2Stream::SetPriority(uint32_t newGroup)

of course after writing this I moved onto draft 12, and in draft 12 we end up with everything dependent on stream 0 using the nice-like weight and the non-exclusive bit.. and that's pretty much exactly what I propose above.

so assuming we land 11 and 12 together, this is moot.
Attachment #8404352 - Flags: review+
I'm fine with landing with draft 12 (leapfrog!). FWIW, my intention was to implement the grouped priority exactly the way you suggested, but I guess somewhere along the line I either changed my mind (doubtful) or just got confused (most likely) and did the wrong thing. I'll add the logging statements and bounds checks, and we'll call this ready to land (when draft 12 lands).
Actually, all the log statements will just disappear in draft 12, so I'm not even going to bother with those :) Bounds checks will still be relevant, though, and I'll just add those on top of draft 12 to avoid rebase hell :)
https://hg.mozilla.org/mozilla-central/rev/1f052f973ee1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Fat-fingered the bug number in the commit message for part 2. Here's the m-c landing of that https://hg.mozilla.org/mozilla-central/rev/7e0fe7535587
You need to log in before you can comment on or make changes to this bug.