1377208 - Move background transactions over h2 to the background group

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

+rise priority of the background group by one.  This will send an update for each transaction to the server, but we can coalesce by buffering so that there is a good chance these updates go out in a single TCP frame.

Comment 1

[Honza Bambas (:mayhemer)]

(Find an owner)

Comment 2

[Shian-Yow Wu [:swu]]

Kershaw will look at it.

Comment 3

[Honza Bambas (:mayhemer)]

(Forgot to press enter when I was writing this comment!)

Kershaw, I was just talking to :swu and he told me you could take this bug.  I've got some pointers from Nick (will forward), but they more say what we can't do than what we can regarding this issue.  

I believe you will have to dive into the http/2 code a bit and understand how the priorities, dependencies and trees in h2 actually work.

Unfortunately, both of our h2 experts Nick and Patrick (somewhat) are unreachable at the moment.  Nick is on vacation and Patrick seems to be occupied lately.

Thanks, let me know how I can be of any help.

Comment 4

[Kershaw Chang [:kershaw]]

Honza,

Could you explain a bit more about the goal of this bug? Sorry that it's difficult for me to understand it clearly from comment#0.

From this bug's summary, it looks like we just have to add nsIClassOfService::Background to background transactions, but my gut tells me it's not that simple.

Comment 5

[Honza Bambas (:mayhemer)]

I think the description is already obsolete, sorry I missed that and didn't update.

The goal here is, as I understand it: transactions for background tabs should be put to a new priority group, let's call it 'background'.  I still don't fully understand if the group has to be part of a dependency tree (among other groups we already have?) or has to have a different weight (which is h2 synonym for a priority?).

This is something to find out (ask Patrick once more, sorry :)) or simply look into the h2 code and maybe specs how we deal with this stuff.

Comment 6

[u408661]

So we already have a 'background' node in the priority tree: https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/netwerk/protocol/http/Http2Session.h#176

So, any channel (which eventually maps to an Http2Stream) that gets nsIClassOfService::Background should be a child (dependent) of that node in the priority tree.

We have examples of generating child nodes around https://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/netwerk/protocol/http/Http2Session.cpp#955 (nb: you shouldn't use CreatePriorityNode for this work, as it's specific to our "dummy" nodes that form the basis of our priority tree).

To clarify about dependency vs. weight... they're related. At each level of the priority tree, the individual nodes are weighted with respect to each other. I... really honestly need a visual to explain this well, and I suck at ascii art, so that's about all the explanation you'll get for now (until I can draw & scan something). Suffice to say, you can probably (at least for the first run at this) leave the weight alone when you move a channel into the background group. There are certainly more interesting things we could do, but let's get it working first, then we can fiddle with the details :)

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: WIP

Hi Nick,

Could you please take a look at this wip patch? I'd like to know if I am on the right track.

Basically, this patch does two things:
1. When a new stream is created, make it depend on kBackgroundGroupID if stream's transaction is from inactive tabs.
2. Update each stream's dependency when active tab is changed.

Thanks.

Comment 8

[u408661]

Comment on attachment 8898659 [details] [diff] [review]
WIP

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

Thanks, Kershaw. This is on the right track, but needs (I think) a little rejiggering to match a bit better with how things are generally done in h2, and to simplify things a bit.

::: netwerk/protocol/http/Http2Session.cpp
@@ +4474,5 @@
> +  // Check whether each stream's dependency needs to be updated because
> +  // the active tab is changed.
> +  for (auto iter = mStreamTransactionHash.Iter(); !iter.Done(); iter.Next()) {
> +    nsAutoPtr<Http2Stream> &stream = iter.Data();
> +    uint32_t priorityDependency = stream->PriorityDependency();

I think I'd rather you just keep track of this all in the stream, instead of the session. Heck, the stream can even call CreatePriorityNode itself (from inside UpdatePriorityDependency), that way you don't need to worry about checking anything here.

::: netwerk/protocol/http/Http2Session.h
@@ +255,5 @@
>  
>    // For use by an HTTP2Stream
>    void Received421(nsHttpConnectionInfo *ci);
>  
> +  void TopLevelOuterContentWindowIdChanged() override final;

This should probably be declared & stubbed out in nsAHttpConnection, then overridden in the appropriate places. (Its override declaration should be put in NS_DECL_NSAHTTPCONNECTION)

::: netwerk/protocol/http/Http2Stream.h
@@ +171,5 @@
>    nsresult GetOriginAttributes(mozilla::OriginAttributes *oa);
>  
> +  uint32_t PriorityDependency() { return mPriorityDependency; }
> +
> +  uint8_t PriorityWeight() { return mPriorityWeight; }

You can get rid of these if you do all the work in the stream, instead of the session.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +243,5 @@
>      bool IsConnEntryUnderPressure(nsHttpConnectionInfo*);
>  
> +    uint64_t CurrentTopLevelOuterContentWindowId()
> +    {
> +        return mCurrentTopLevelOuterContentWindowId;

Instead of adding this accessor, why not just pass mCurrentTopLevelOuterContentWindowId as an argument into conn->TopLevelOuterContentWindowIdChanged? One less thing for consumers to have to look up.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: patch - v2

Thanks for the feedback!

> 
> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +4474,5 @@
> > +  // Check whether each stream's dependency needs to be updated because
> > +  // the active tab is changed.
> > +  for (auto iter = mStreamTransactionHash.Iter(); !iter.Done(); iter.Next()) {
> > +    nsAutoPtr<Http2Stream> &stream = iter.Data();
> > +    uint32_t priorityDependency = stream->PriorityDependency();
> 
> I think I'd rather you just keep track of this all in the stream, instead of
> the session. Heck, the stream can even call CreatePriorityNode itself (from
> inside UpdatePriorityDependency), that way you don't need to worry about
> checking anything here.
> 

Done. But, I have to add "friend class Http2Stream;" in Http2Session so that Http2Stream can call CreatePriorityNode().

> ::: netwerk/protocol/http/Http2Session.h
> @@ +255,5 @@
> >  
> >    // For use by an HTTP2Stream
> >    void Received421(nsHttpConnectionInfo *ci);
> >  
> > +  void TopLevelOuterContentWindowIdChanged() override final;
> 
> This should probably be declared & stubbed out in nsAHttpConnection, then
> overridden in the appropriate places. (Its override declaration should be
> put in NS_DECL_NSAHTTPCONNECTION)
> 

Sorry, I didn't make this change. I think the best path to notify http2 stream of window id is something like:
nsHttpConnectionMgr->GetSpdyActiveConn->mSpdySession->TopLevelOuterContentWindowIdChanged(Http2Session)->TopLevelOuterContentWindowIdChanged(Http2Stream)

If I make "TopLevelOuterContentWindowIdChanged" be declared in nsAHttpConnection, I have no idea how to pass window id from nsHttpConnectionMgr to Http2Stream.

> ::: netwerk/protocol/http/Http2Stream.h
> @@ +171,5 @@
> >    nsresult GetOriginAttributes(mozilla::OriginAttributes *oa);
> >  
> > +  uint32_t PriorityDependency() { return mPriorityDependency; }
> > +
> > +  uint8_t PriorityWeight() { return mPriorityWeight; }
> 
> You can get rid of these if you do all the work in the stream, instead of
> the session.
> 

Done.

> ::: netwerk/protocol/http/nsHttpConnectionMgr.h
> @@ +243,5 @@
> >      bool IsConnEntryUnderPressure(nsHttpConnectionInfo*);
> >  
> > +    uint64_t CurrentTopLevelOuterContentWindowId()
> > +    {
> > +        return mCurrentTopLevelOuterContentWindowId;
> 
> Instead of adding this accessor, why not just pass
> mCurrentTopLevelOuterContentWindowId as an argument into
> conn->TopLevelOuterContentWindowIdChanged? One less thing for consumers to
> have to look up.

I still keep this in nsHttpConnectionMgr because I need to know the window id when a new http2 stream is created.

Please take a look at this patch. Thanks.

Comment 10

[u408661]

Comment on attachment 8899420 [details] [diff] [review]
patch - v2

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

OK, coming back to this with fresh eyes, I'm seeing more issues this time around. This is still heading on the right track, though!

I'm still a fan of having TopLevelOuterContentWindowIdChanged be a member of nsAHttpConnection (and also of Http2Stream - that's how you'd get it to the stream), especially since it's possible (though I don't know if it's guaranteed) that we'll have another nsAHttpConnection implementer in the near future (quic-related) that will likely want to know that information (better to make it fail at compile time).

::: netwerk/protocol/http/Http2Push.cpp
@@ +63,5 @@
>  Http2PushedStream::Http2PushedStream(Http2PushTransactionBuffer *aTransaction,
>                                       Http2Session *aSession,
>                                       Http2Stream *aAssociatedStream,
>                                       uint32_t aID)
> +  :Http2Stream(aTransaction, aSession, 0, 0)

I think you want to change this to have the appropriate window id - pushed streams use the same priority dependency calculation code as pulled streams. It might not actually matter, but it's not significantly more difficult, and it's safer in case we change things in the future.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +72,5 @@
>    , mTotalSent(0)
>    , mTotalRead(0)
>    , mPushSource(nullptr)
>    , mAttempting0RTT(false)
> +  , mCurrentTopLevelOuterContentWindowId(windowId)

I'd rather call this something like "mCurrentForegroundTabOuterContentWindowId" - otherwise the naming is too confusingly similar to the name of the method we use to get the window id of our tab's content window (regardless of if it's in the foreground or not).

@@ +1215,5 @@
>    if (!trans) {
>      return;
>    }
>  
> +  if (trans->TopLevelOuterContentWindowId() != mCurrentTopLevelOuterContentWindowId) {

trans->TopLevelOuterContentWindowId() shouldn't ever change, right? If that's the case, we should just cache the value in the ctor and not bother calling into the transaction here.

@@ +1220,5 @@
> +    LOG3(("Http2Stream::UpdatePriorityDependency %p "
> +          " depends on background group for trans %p\n",
> +          this, trans));
> +    mPriorityDependency = Http2Session::kBackgroundGroupID;
> +    return;

Hrm... on second view, this short-circuit worries me a little bit. I would rather we put this below, where we make the determination based on class flags. Partially because this overrides urgent-start (which I believe it is possible to have set on something from a background tab). I'm not totally clear on which should take priority for the determination here, we should ask Honza.

@@ +1276,5 @@
> +
> +  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
> +  if (!trans) {
> +    return;
> +  }

This block isn't necessary, you never use trans in this method.

@@ +1279,5 @@
> +    return;
> +  }
> +
> +  uint32_t currentPriorityDependency = mPriorityDependency;
> +  UpdatePriorityDependency();

Again, the ordering in the above worries me. I'm not sure exactly what should take precedence (and it's possible that what takes precedence should be different at different points in the lifecycle of the stream). Again, this is a question for Honza. (For example, we may want to use a different/new method here, instead of relying on the same stuff.) (Part of the problem here is that "UpdatePriorityDependency" has never been used to "update" anything - it sets the dependency at the beginning of the stream, and is never called afterwards.)

Ideally, I think, you would not even call UpdatePriorityDependency here - just check to see if we're background or not, and send the appropriate frame.

@@ +1282,5 @@
> +  uint32_t currentPriorityDependency = mPriorityDependency;
> +  UpdatePriorityDependency();
> +
> +  if (currentPriorityDependency != mPriorityDependency) {
> +    mSession->CreatePriorityNode(mStreamID,

Like I said in my initial comment, CreatePriorityNode (specifically the logging inside it) is specific to our original priority tree. What you're doing here won't make sense in the logs. (We aren't even "creating" a node here, we're reprioritizing one!) You should probably refactor CreatePriorityNode into a method that creates the frame, and two methods that log/drive frame creation - one for the node creation as we have now, and one for this update of an existing stream's priority. The updating method can live in the stream (it can probably be just a log statement here, and then a call through to the method on the stream that creates a frame with no logging).

Also, don't make the stream a friend of the session. Just make the method that creates a PRIORITY frame public (like all the other session methods we use from within the stream).

@@ +1287,5 @@
> +                                 mPriorityDependency,
> +                                 mPriorityWeight,
> +                                 "");
> +    mSession->FlushOutputQueue();
> +  }

So right now, once a stream gets shoved into the background group because of the tab ordering, it will never get out of the background group, regardless of if its tab gets foregrounded or not. That's bad. You need another case here to restore its original dependency.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: patch - v3

Sorry for the late update. Please take a look at this one.

Summary:
 - Add TopLevelOuterContentWindowIdChanged in nsAHttpConnection.
 - Pass currentForegroundTabOuterContentWindowId into Http2PushedStream.
 - Cache the value of trans->TopLevelOuterContentWindowId().
 - Add a helper function in Http2Stream to get priority dependency from a transaction.
 - Refactor Http2Session::CreatePriorityNode.
 - Add NotifyConnectionOfWindowIdChange() in nsHttpConnectionMgr for sending a notification to all connections.

In this patch, I still make active tab priority override urgent start, but I am not 100% sure. I'll ask Honza about this.

Thanks.

Comment 12

[Kershaw Chang [:kershaw]]

Honza, could you please take a look at comment #10?

The problem is that what should we deal with an urgent start h2 stream from background tab? I think we should still make it depend on background stream (This is a bit easier to implement). What do you think?

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8901139 [details] [diff] [review]
patch - v3

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

::: netwerk/protocol/http/Http2Stream.cpp
@@ +105,5 @@
>    SetPriority(static_cast<uint32_t>(httpPriority));
> +
> +  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
> +  if (trans) {
> +    mTransactionTabId = trans->TopLevelOuterContentWindowId();

note that this caching, as Nick suggested, only saves one dereference and takes 8 bytes of memory per Http2Stream object.

@@ +1227,5 @@
> +    return Http2Session::kBackgroundGroupID;
> +  } else if (classFlags & nsIClassOfService::Unblocked) {
> +    return Http2Session::kOtherGroupID;
> +  } else if (classFlags & nsIClassOfService::UrgentStart) {
> +    return Http2Session::kUrgentStartGroupID;

nits:
- three are tuns of else after return
- shouldn't the order be UrgentStart, Leader, Unblocked, Follower, Specul, Background?  the flags can be combined

@@ +1271,1 @@
>      mPriorityDependency = Http2Session::kBackgroundGroupID;

Ah, now I get the questions in comment 10!  I think the urgent start group should be left, yes.  good point!  because if you open a background tab, we want the top level document load quickly regardless it's in the background tab.  this is how it works for h1 - urgent start takes an absolute precedence.

Note that urgent start was designed to give a higher priority to stuff that is invoked from a user interaction.  Hence, doing this only for the top level loads in background tabs is OK.  And it's hard to interact with a background tab, the last time I checked ;)

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +144,5 @@
>  
>      // nsHttp.h version
>      virtual uint32_t Version() = 0;
> +
> +    // An notification of the current active tab id change.

A notification

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3414,5 @@
>  
>  void
> +nsHttpConnectionMgr::NotifyConnectionOfWindowIdChange(uint64_t previousWindowId)
> +{
> +    nsTArray<RefPtr<nsHttpTransaction>> *transactions = nullptr;

please assert socket thread when you are accessing the mActiveTransactions array.

@@ +3432,5 @@
> +            }
> +        };
> +
> +    for (uint32_t i = 0; i < 2; i++) {
> +        bool throttled = i % 2 == 0;

!!i ? OTOH, inlining this w/o a loop may be better, IMO

@@ +3438,5 @@
> +        addConnectionHelper(transactions);
> +
> +        transactions =
> +            mActiveTransactions[throttled].Get(mCurrentTopLevelOuterContentWindowId);
> +        addConnectionHelper(transactions);

smart!  I like that you notify only actually affected transactions.

Comment 14

[Honza Bambas (:mayhemer)]

I thin I answered comment 10 above.

Comment 15

[u408661]

Comment on attachment 8901139 [details] [diff] [review]
patch - v3

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

::: netwerk/protocol/http/Http2Session.cpp
@@ +802,5 @@
>    MOZ_ASSERT(OnSocketThread(), "not on socket thread");
>    LOG3(("Http2Session::GeneratePriority %p %X %X\n",
>          this, aID, aPriorityWeight));
>  
> +  char *packet = EnsureOutputBuffer(kFrameHeaderBytes + 5);

This EnsureOutputBuffer should be in CreatePriorityFrame, not here (same with similar calls in other spots of the patch)

::: netwerk/protocol/http/Http2Stream.cpp
@@ +1227,5 @@
> +    return Http2Session::kBackgroundGroupID;
> +  } else if (classFlags & nsIClassOfService::Unblocked) {
> +    return Http2Session::kOtherGroupID;
> +  } else if (classFlags & nsIClassOfService::UrgentStart) {
> +    return Http2Session::kUrgentStartGroupID;

What Honza said about the else after return - totally not needed, just use bare ifs.

As for the ordering... it should be kept the way it was in the original *except* for urgentstart - that should be moved to the beginning.

@@ +1294,5 @@
> +    LOG3(("Http2Stream::TopLevelOuterContentWindowIdChanged %p "
> +          "move into background group.\n", this));
> +
> +    mSession->SendPriorityFrame(mStreamID, mPriorityDependency, mPriorityWeight);
> +    return;

Please don't early-return, just have an if/else that sets dependency appropriately, then call to SendPriorityFrame after.

@@ +1299,5 @@
> +  }
> +
> +  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
> +  if (!trans) {
> +    return;

(This early return, however, is ok.)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3432,5 @@
> +            }
> +        };
> +
> +    for (uint32_t i = 0; i < 2; i++) {
> +        bool throttled = i % 2 == 0;

Yes, like Honza said, please inline this. Right now, it's just hard to read.

Comment 16

[Kershaw Chang [:kershaw]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #15)
> Comment on attachment 8901139 [details] [diff] [review]
> patch - v3
> 
> Review of attachment 8901139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +802,5 @@
> >    MOZ_ASSERT(OnSocketThread(), "not on socket thread");
> >    LOG3(("Http2Session::GeneratePriority %p %X %X\n",
> >          this, aID, aPriorityWeight));
> >  
> > +  char *packet = EnsureOutputBuffer(kFrameHeaderBytes + 5);
> 
> This EnsureOutputBuffer should be in CreatePriorityFrame, not here (same
> with similar calls in other spots of the patch)
> 

CreatePriorityFrame is also used in CreatePriorityNode, which means EnsureOutputBuffer will be called multiple times if I move it into CreatePriorityFrame. Note that before calling CreatePriorityNode, EnsureOutputBuffer is already called at [1].
Not sure if it's safe to call EnsureOutputBuffer again.

Anyway, I'll update the patch as you suggested.

[1] https://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/netwerk/protocol/http/Http2Session.cpp#881

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: patvh - v4

Summary:
 - Fix review comments.
 - Please also see comment#16.

Thanks.

Comment 18

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> Comment on attachment 8901139 [details] [diff] [review]
> patch - v3
> 
> Review of attachment 8901139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/Http2Stream.cpp
> @@ +105,5 @@
> >    SetPriority(static_cast<uint32_t>(httpPriority));
> > +
> > +  nsHttpTransaction *trans = mTransaction->QueryHttpTransaction();
> > +  if (trans) {
> > +    mTransactionTabId = trans->TopLevelOuterContentWindowId();
> 
> note that this caching, as Nick suggested, only saves one dereference and
> takes 8 bytes of memory per Http2Stream object.
> 

Not sure if it's worth to cache this.
Nick, could you reconsider this? Thanks.

Comment 19

[u408661]

Comment on attachment 8901716 [details] [diff] [review]
patvh - v4

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

Sorry for the lag here... this looks good! A few minor nits for you to fix before landing, but first a couple general comments:

(In reply to Kershaw Chang [:kershaw] from comment #16)
> CreatePriorityFrame is also used in CreatePriorityNode, which means
> EnsureOutputBuffer will be called multiple times if I move it into
> CreatePriorityFrame. Note that before calling CreatePriorityNode,
> EnsureOutputBuffer is already called at [1].
> Not sure if it's safe to call EnsureOutputBuffer again.

Yes, calling EnsureOutputBuffer multiple times is perfectly safe - it eventually becomes https://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/netwerk/protocol/http/nsHttp.cpp#547 (which will be a noop when called twice in quick succession).

(In reply to Kershaw Chang [:kershaw] from comment #18)
> Not sure if it's worth to cache this.
> Nick, could you reconsider this? Thanks.

What's funny is, if it had been cached in the first place, the chances of anyone asking you to switch to getting the info at the usage site is pretty slim (if not nonexistent). Let's keep it the way it is - 8 more bytes per stream isn't going to make a huge difference.

::: netwerk/protocol/http/Http2Stream.cpp
@@ +1210,5 @@
>          exclusive));
>  }
>  
> +static uint32_t
> +GetPriorityDependencyFromTransactionHelper(nsHttpTransaction *trans)

nit: drop the "Helper" here. They're all helpers in some way or another :) (Also, "FooHelper" sounds too enterprise-Java-esque.)

@@ +1219,5 @@
> +
> +  if (classFlags & nsIClassOfService::UrgentStart) {
> +    return Http2Session::kUrgentStartGroupID;
> +  }
> +

nit: you have a blank line between this if and the rest, but not between the rest of the ifs. I'd prefer a blank line between each one, but whichever you pick, please be consistent :)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3433,5 @@
> +                }
> +            }
> +        };
> +
> +    transactions = mActiveTransactions[false].Get(previousWindowId);

Please note here what false & true represent. It's not obvious from reading the method.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +763,5 @@
>  
>      nsTArray<RefPtr<PendingTransactionInfo>>*
>      GetTransactionPendingQHelper(nsConnectionEntry *ent, nsAHttpTransaction *trans);
>  
> +    // When current active tab is changed, this function use

nit: "uses"

@@ +764,5 @@
>      nsTArray<RefPtr<PendingTransactionInfo>>*
>      GetTransactionPendingQHelper(nsConnectionEntry *ent, nsAHttpTransaction *trans);
>  
> +    // When current active tab is changed, this function use
> +    // |previousWindowId| to select background transactions and use

nit: you can drop this "use" entirely

@@ +766,5 @@
>  
> +    // When current active tab is changed, this function use
> +    // |previousWindowId| to select background transactions and use
> +    // mCurrentTopLevelOuterContentWindowId| to select foreground transactions.
> +    // Then, notify selected transactions' connection of current active tab id.

nit (two changes here): "Then, it notifies [...] of the new active tab id."

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: patch for landing

Summary:
 - Fix nits.
 - Carry r+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1533800f2cc168615111b12ab95cdbb19b4b8b00

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e3ab3bb127
Move background h2 stream into background group. r=hurley

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f8e3ab3bb127

Comment 23

[Honza Bambas (:mayhemer)]

Kershaw, is there some preference that would control this feature?

This patch seems to have a visible positive impact!  I would like to have a preference to prove it via a synthetic test.  Thanks.

Comment 24

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> Kershaw, is there some preference that would control this feature?
> 
> This patch seems to have a visible positive impact!  I would like to have a
> preference to prove it via a synthetic test.  Thanks.

I also use "network.http.active_tab_priority" for this feature.