telemetry indicates we are creating lots more http2sessions with coalescing rework

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: HTTP
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [necko-active][spdy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months ago
https://bugzilla.mozilla.org/show_bug.cgi?id=1337791 changed how connection manager implements rfc 7540 coalescing - it also implements the ORIGIN frame but I don't think that has anything to do with this bug as I don't believe any server is sending it.

that code landed on april 4 on gecko-55. after that time the number of http2session telemetry reports approximately doubled - most of those reports are made once per h2session.

from the impact on the telemetry distributions it appears these sessions often don't do anything (i.e. they push down the avg number of streams per session, they are less likely to end in an error state, etc..)

chris h-c reported a lot of useful data:

https://bugzilla.mozilla.org/show_bug.cgi?id=1353497#c8
https://bugzilla.mozilla.org/show_bug.cgi?id=1353497#c9
https://bugzilla.mozilla.org/show_bug.cgi?id=1353497#c10
(Assignee)

Updated

11 months ago
Summary: telemetry indicates we are creating lots more httpsessions with coalescing rework → telemetry indicates we are creating lots more http2sessions with coalescing rework
(Assignee)

Comment 1

11 months ago
its not impossible this is working in a reasonable way - but it should be understood.
(Assignee)

Updated

11 months ago
Blocks: 1353497
No longer blocks: 1337791
No longer depends on: 1353497
(Assignee)

Updated

11 months ago
Blocks: 1337791
No longer blocks: 1353497
(Assignee)

Updated

11 months ago
Assignee: nobody → mcmanus
(Assignee)

Updated

11 months ago
See Also: → bug 1355875
(Assignee)

Comment 2

11 months ago
I think I get it. We're creating a bunch of connections where we didn't used to - mostly out of OnMsgSpeculativeConnect(). That fx() checks to see if there are any active connections and if so doesn't make more..

in the pre-origin code in the presence of coalescing it checked a forwarded connection entry for that information.

in the post-origin world we don't forward connection entries anymore - so the limit isn't located. however when the new connection is setup we immediately realize we have an old conn in the hashtable that can service it (from a different conn entry) so we close it down. and we end up with a bunch of stray h2 connections that don't ever do anything - which also show up as really short and uninteresting telemetry entries.

this is pretty easy to fix.
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Comment hidden (mozreview-request)
(Assignee)

Comment 4

11 months ago
mozreview-review
Comment on attachment 8857940 [details]
Bug 1355591 - h2 coalescing creating unused sessions

https://reviewboard.mozilla.org/r/129972/#review132540

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:963
(Diff revision 1)
> -         "queued=%" PRIuSIZE "]\n",
> +         " queued=%" PRIuSIZE "]\n",
>           ent->mConnInfo->HashKey().get(), ent, ent->mActiveConns.Length(),
>           ent->mIdleConns.Length(), ent->mUrgentStartQ.Length(),
>           ent->PendingQLength()));
>  
> -    if (!ent->mUrgentStartQ.Length() && !ent->mPendingTransactionTable.Count()) {
> +    if (!ent->mUrgentStartQ.Length() && !ent->PendingQLength()) {

this is a ridealong fixing code in the original origin patchset.. note that it now matches the log line above it. the buggy version was harmless, it just didn't catch all the 0 conditions it was trying to short circuit.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:1152
(Diff revision 1)
>  bool
>  nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent)
>  {
>      MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>  
> +    if (ent->AvailableForDispatchNow()) {

this is the real fix
(Assignee)

Comment 5

11 months ago
I would expect our number of submissions to go down after this patch, perhaps even to lower levels than before apr 6 due to better coalescing of certain cases.
Comment hidden (mozreview-request)
Comment on attachment 8857940 [details]
Bug 1355591 - h2 coalescing creating unused sessions

https://reviewboard.mozilla.org/r/129972/#review132564
Attachment #8857940 - Flags: review?(hurley) → review+

Comment 8

11 months ago
Pushed by mcmanus@ducksong.com:
https://hg.mozilla.org/integration/autoland/rev/a405331211ad
h2 coalescing creating unused sessions r=nwgh

Comment 9

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a405331211ad
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.