Closed Bug 1341266 Opened 3 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: amchung, Assigned: xeonchen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

Set the highest priority when the transaction marked urgent-start on Http2Session.cpp.
Whiteboard: [necko-active]
Attached patch implementation (obsolete) — Splinter Review
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!
Attachment #8847434 - Flags: feedback?(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?
Attachment #8847434 - Flags: feedback?(mcmanus)
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!
Flags: needinfo?(mcmanus)
Attached patch implementation (obsolete) — Splinter Review
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!
Attachment #8847434 - Attachment is obsolete: true
Flags: needinfo?(mcmanus)
Attachment #8855282 - Flags: feedback?(mcmanus)
Priority: -- → P1
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?
Attachment #8855282 - Flags: feedback?(mcmanus)
(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.
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.
Assignee: amchung → nobody
Gary, can you please take this bug?  Thanks.  Patrick is your source of information here ;)
Assignee: nobody → xeonchen
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #8855282 - Attachment is obsolete: true
Attachment #8869087 - Flags: feedback?(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.
Attachment #8869087 - Flags: feedback?(mcmanus)
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!
Flags: needinfo?(honzab.moz)
(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!
Flags: needinfo?(honzab.moz)
Attachment #8869087 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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 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.
Attachment #8870759 - Flags: review?(mcmanus) → review+
Pushed by gachen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2639c72118b4
create http/2 stream for urgent-start; r=mcmanus
https://hg.mozilla.org/mozilla-central/rev/2639c72118b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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.
You need to log in before you can comment on or make changes to this bug.