1341266 - Set the highest priority when the transaction marked urgent-start on http2.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Amy Chung [:Amy]]

Set the highest priority when the transaction marked urgent-start on Http2Session.cpp.

Comment 1

[Amy Chung [:Amy]]

Attachment: implementation

Hi Patrick,
I have implemented the urgent-start function on h2:
1. Created a new group id "kUrgentStartGroupID".
2. Set mPriorityDependency = Http2Session::kUrgentStartGroupID when classFlag = UrgentStart on UpdatePriorityDependency().
3. Set the class service to nsIClassOfService::UrgentStart on Http2Session::AddStream().

Would you give me some suggestions on my patch?
Thanks!

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8847434 [details] [diff] [review]
implementation

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

you call this urgent start - but it seems to maintain the whole priority the whole time (i.e. not just start).. should you instead be reprioritizing this after it gets going? I think so..

::: netwerk/protocol/http/Http2Session.cpp
@@ +961,5 @@
>      mNextStreamID += 2;
>      MOZ_ASSERT(mNextStreamID == kFollowerGroupID);
>      CreatePriorityNode(kFollowerGroupID, kLeaderGroupID, 0, "follower");
>      mNextStreamID += 2;
> +    MOZ_ASSERT(mNextStreamID == kUrgentStartGroupID);

given that urgent is typo'd to be 0xe instead of 0xd below, I'm concerned that you've never run this code or this assert would have fired immediately.

@@ +962,5 @@
>      MOZ_ASSERT(mNextStreamID == kFollowerGroupID);
>      CreatePriorityNode(kFollowerGroupID, kLeaderGroupID, 0, "follower");
>      mNextStreamID += 2;
> +    MOZ_ASSERT(mNextStreamID == kUrgentStartGroupID);
> +    CreatePriorityNode(kUrgentStartGroupID, 0, 255, "urgentstart");

this is really extreme. you might want to dial it back closer to leader status.

::: netwerk/protocol/http/Http2Session.h
@@ +169,5 @@
>      kOtherGroupID =       0x5,
>      kBackgroundGroupID =  0x7,
>      kSpeculativeGroupID = 0x9,
> +    kFollowerGroupID =    0xB,
> +    kUrgentStartGroupID = 0xE,

why wouldn't this be 0xd?

Comment 3

[Amy Chung [:Amy]]

Hi Patrick,
I have some question about setting the highest priority when the transaction marked urgent-start on http2 as below:
1. Do I change the priority when the channel be marked to urgent start?
   i. Depend on 1348044, 1348050 and 1348053.
2. Do I just need to create new group id "kUrgentStartGroupID" now?

Would you give me some suggestions?
Thanks!

Comment 4

[Amy Chung [:Amy]]

Attachment: implementation

Hi Patrick,
[The steps of my implementation]
1. Created a new group id "kUrgentStartGroupID".
2. Set mPriorityDependency = Http2Session::kUrgentStartGroupID when classFlag = UrgentStart on UpdatePriorityDependency().
3. Roll-backed the priority to normal when channel have to clear the urgent-sart of calssOfService.

[Modification]
1. Modified kUrgentStartGroupID to 0xD on Http2Session.h.

Would you give me some suggestions on my patch?
Thanks!

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8855282 [details] [diff] [review]
implementation

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

::: netwerk/protocol/http/Http2Session.cpp
@@ +964,3 @@
>      // Hey, you! YES YOU! If you add/remove any groups here, you almost
>      // certainly need to change the lookup of the stream/ID hash in
>      // Http2Session::OnTransportStatus. Yeah, that's right. YOU!

you didn't read this comment.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +1199,5 @@
>    // 5 depends 0, weight 100, other (kOtherGroupID)
>    // 7 depends 0, weight 0, background (kBackgroundGroupID)
>    // 9 depends 7, weight 0, speculative (kSpeculativeGroupID)
>    // b depends 3, weight 0, follower class (kFollowerGroupID)
> +  // e depends 0, weight 255, urgent-start class (kUrgentStartGroupID)

its d not e

as I said before 255 is pretty extreme. is that what you want?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6394,5 @@
>  nsHttpChannel::ClearClassFlags(uint32_t inFlags)
>  {
> +    if (inFlags & nsIClassOfService::UrgentStart) {
> +      // Revert priority when clear urgent-start flags.
> +      SetPriority(nsISupportsPriority::PRIORITY_NORMAL);

this is a start, but I don't think that actually turn into a re-pri frame on the network.. it looks like it deadends in nsHttpTransaction. have you tested it?

Comment 6

[Amy Chung [:Amy]]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8855282 [details] [diff] [review]
> implementation
> 
> Review of attachment 8855282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +964,3 @@
> >      // Hey, you! YES YOU! If you add/remove any groups here, you almost
> >      // certainly need to change the lookup of the stream/ID hash in
> >      // Http2Session::OnTransportStatus. Yeah, that's right. YOU!
> 
> you didn't read this comment.
> 
Ok, I will modify it.

> ::: netwerk/protocol/http/Http2Stream.cpp
> @@ +1199,5 @@
> >    // 5 depends 0, weight 100, other (kOtherGroupID)
> >    // 7 depends 0, weight 0, background (kBackgroundGroupID)
> >    // 9 depends 7, weight 0, speculative (kSpeculativeGroupID)
> >    // b depends 3, weight 0, follower class (kFollowerGroupID)
> > +  // e depends 0, weight 255, urgent-start class (kUrgentStartGroupID)
> 
> its d not e
> 
> as I said before 255 is pretty extreme. is that what you want?
> 
Sorry for I didn't modify the comment.
If set weight to 255 will probably cause the problem, I will set mPriorityDependency to kLeaderGroupID.

> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +6394,5 @@
> >  nsHttpChannel::ClearClassFlags(uint32_t inFlags)
> >  {
> > +    if (inFlags & nsIClassOfService::UrgentStart) {
> > +      // Revert priority when clear urgent-start flags.
> > +      SetPriority(nsISupportsPriority::PRIORITY_NORMAL);
> 
> this is a start, but I don't think that actually turn into a re-pri frame on
> the network.. it looks like it deadends in nsHttpTransaction. have you
> tested it?
I didn't test it, and I will think about how to improve this patch.

Thanks for your suggestions.

Comment 7

[Amy Chung [:Amy]]

Because I need to spend more time to fix the quantum flow issue https://bugzilla.mozilla.org/show_bug.cgi?id=1331680, have to ask Honza for help to reassign other Necko team member to resolve this issue.

Comment 8

[Honza Bambas (:mayhemer)]

Gary, can you please take this bug?  Thanks.  Patrick is your source of information here ;)

Comment 9

[Gary Chen [:xeonchen]]

Attachment: v1

Not sure if I misunderstood the meaning of urgent-start mechanism, but based on comment 2 I can only guess the priority setting is supposed to be reset after it getting started.

Please correct me if I'm wrong.

Comment 10

[Patrick McManus [:mcmanus]]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #9)
> Created attachment 8869087 [details] [diff] [review]
> v1
> 
> Not sure if I misunderstood the meaning of urgent-start mechanism, but based

There's no way I can review a patch submitted with that :) Talk to honza about the feature first, please.

Comment 11

[Gary Chen [:xeonchen]]

Honza, I've submitted a patch, which does:

1. Add a urgent-start group in Http2Session
2. Make Http2Streams with urgent-start flag set in the urgent-start group.
3. Clear urgent-start flag and reset priority in OnStartRequest

Is this your expected behavior? If not, would you briefly explain your expectation?
Thank you!

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #11)
> Honza, I've submitted a patch, which does:
> 
> 1. Add a urgent-start group in Http2Session
> 2. Make Http2Streams with urgent-start flag set in the urgent-start group.
> 3. Clear urgent-start flag and reset priority in OnStartRequest

Except the (3) I agree.  I would like to experiment with leaving urgent requests in the higher priority group for h2.  I have at least one site that suffers from either misconfigured server or us giving bad priority hints.  And this might fix it nicely.

> 
> Is this your expected behavior? If not, would you briefly explain your
> expectation?
> Thank you!

Comment 13

[Gary Chen [:xeonchen]]

Attachment: Bug 1341266 - create http/2 stream for urgent-start;

Review commit: https://reviewboard.mozilla.org/r/142246/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142246/

Comment 14

[Gary Chen [:xeonchen]]

Comment on attachment 8870759 [details]
Bug 1341266 - create http/2 stream for urgent-start;

Hi Patrick, I've modified the patch after discussion with Honza, and currently the urgent-start priority will not be reset.

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8870759 [details]
Bug 1341266 - create http/2 stream for urgent-start;

https://reviewboard.mozilla.org/r/142246/#review149310

my concern here remains the same. transactions with this flag are just going to get most of the bandwidth until they are done.. its the 'until they are done' part that doesn't seem to match the start part of "urgenet-start". We should adjust it later.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2639c72118b4
create http/2 stream for urgent-start; r=mcmanus

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2639c72118b4

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #15)
> Comment on attachment 8870759 [details]
> Bug 1341266 - create http/2 stream for urgent-start;
> 
> https://reviewboard.mozilla.org/r/142246/#review149310
> 
> my concern here remains the same. transactions with this flag are just going
> to get most of the bandwidth until they are done.. its the 'until they are
> done' part that doesn't seem to match the start part of "urgenet-start". We
> should adjust it later.

Yes, if we find out this is a problem, let's add another patch (or use the parts that have already been submitted here) that reverts the priority back after the response starts coming.  I wanted to experiment with keeping the priority what the experience is going to be.