Closed Bug 1355591 Opened 7 years ago Closed 7 years ago

telemetry indicates we are creating lots more http2sessions with coalescing rework

Categories

(Core :: Networking: HTTP, defect)

50 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

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

Attachments

(1 file)

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
Summary: telemetry indicates we are creating lots more httpsessions with coalescing rework → telemetry indicates we are creating lots more http2sessions with coalescing rework
its not impossible this is working in a reasonable way - but it should be understood.
Blocks: 1353497
No longer blocks: 1337791
No longer depends on: 1353497
Blocks: 1337791
No longer blocks: 1353497
Assignee: nobody → mcmanus
See Also: → 1355875
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.
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
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 on attachment 8857940 [details]
Bug 1355591 - h2 coalescing creating unused sessions

https://reviewboard.mozilla.org/r/129972/#review132564
Attachment #8857940 - Flags: review?(hurley) → review+
Pushed by mcmanus@ducksong.com:
https://hg.mozilla.org/integration/autoland/rev/a405331211ad
h2 coalescing creating unused sessions r=nwgh
https://hg.mozilla.org/mozilla-central/rev/a405331211ad
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: