Telemetry data for speculative connections should be specific to Predictor

RESOLVED FIXED in mozilla33

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaypoulz, Assigned: jaypoulz)

Tracking

unspecified
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The telemetry data we've been gathering does not provide us with the information that we want - namely how effect are the Predictor's speculative connections. We should refactor this to be more Predictor specific.
Posted patch bug-1037184-fix.patch (obsolete) — Splinter Review
*** A Few Notes ***

-- Data From Last Test --

Total Predictor Preconnects = 168
Total Sockets Actually Created For These = 60
Total Sockets Create That Were Used = 1
Total Sockets Closed Without Being Used = 59

Total Number of Speculative Connections Used = 353

-- Conclusions --

Who is creating all these spec connections if not us? (nsIOService?)
Do their spec connections take precedence over ours?
Is there a race condiction here?

*** Try build ***

https://tbpl.mozilla.org/?tree=Try&rev=d2d90c4d05ad
Attachment #8454064 - Flags: review?(hurley)
Posted patch bug-1037184-fix.patch (obsolete) — Splinter Review
The last patch made try upset...
https://tbpl.mozilla.org/?tree=Try&rev=75de66b5692a
Attachment #8454064 - Attachment is obsolete: true
Attachment #8454064 - Flags: review?(hurley)
Attachment #8454112 - Flags: review?(hurley)
Posted patch bug-1037184-fix.patch (obsolete) — Splinter Review
Third time is the charm, right?
https://tbpl.mozilla.org/?tree=Try&rev=c7cfb665e7cc
Attachment #8454112 - Attachment is obsolete: true
Attachment #8454112 - Flags: review?(hurley)
Attachment #8454134 - Flags: review?(hurley)
Comment on attachment 8454134 [details] [diff] [review]
bug-1037184-fix.patch

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

::: netwerk/base/public/nsISpeculativeConnect.idl
@@ +61,5 @@
> +    /*
> +     * Used by the Predictor to gather telemetry data on speculative connection
> +     * usage.
> +     */
> +    readonly attribute boolean isFromPredictor;

make this infallible, as well

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +344,5 @@
>  {
>      virtual ~SpeculativeConnectArgs() {}
>  
>  public:
> +    SpeculativeConnectArgs() { mOverridesOK = mIsFromPredictor = false; }

I'm not a fan of the chained assignment. See below for the fix :)

@@ +2067,5 @@
> +
> +        if (isFromPredictor) {
> +          sock->SetIsFromPredictor(true);
> +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> +          ++totalPreconnectsCreated;

Let's move the actual telemetry accumulation to where the predictor actually creates the connection, instead of keeping it in the conn mgr.

We may also want to keep the total speculative connections telemetry, in addition to the seer-specific telemetry. Patrick, what do you think?

@@ +2796,5 @@
>          ((ignoreIdle && (ent->mIdleConns.Length() < parallelSpeculativeConnectLimit)) ||
>           !ent->mIdleConns.Length()) &&
>          !RestrictConnections(ent, ignorePossibleSpdyConnections) &&
>          !AtActiveConnectionLimit(ent, args->mTrans->Caps())) {
> +        CreateTransport(ent, args->mTrans, args->mTrans->Caps(), true, args->mIsFromPredictor);

Instead of just using args->mIsFromPredictor here, use a local value that only gets set if mOverridesOK == true (just like ignoreIdle and friends above). That way we can avoid that ugly chained assignment I complained about above :)
Attachment #8454134 - Flags: review?(hurley)
(In reply to Nicholas Hurley [:hurley] from comment #4)
> @@ +2067,5 @@
> > +
> > +        if (isFromPredictor) {
> > +          sock->SetIsFromPredictor(true);
> > +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> > +          ++totalPreconnectsCreated;
> 
> Let's move the actual telemetry accumulation to where the predictor actually
> creates the connection, instead of keeping it in the conn mgr.
> 
> We may also want to keep the total speculative connections telemetry, in
> addition to the seer-specific telemetry. Patrick, what do you think?

^^^^^
Flags: needinfo?(mcmanus)
(In reply to Nicholas Hurley [:hurley] from comment #5)
> (In reply to Nicholas Hurley [:hurley] from comment #4)
> > @@ +2067,5 @@
> > > +
> > > +        if (isFromPredictor) {
> > > +          sock->SetIsFromPredictor(true);
> > > +          Telemetry::AutoCounter<Telemetry::PREDICTOR_TOTAL_PRECONNECTS_CREATED> totalPreconnectsCreated;
> > > +          ++totalPreconnectsCreated;
> > 
> > Let's move the actual telemetry accumulation to where the predictor actually
> > creates the connection, instead of keeping it in the conn mgr.
> > 
> > We may also want to keep the total speculative connections telemetry, in
> > addition to the seer-specific telemetry. Patrick, what do you think?
> 
> ^^^^^

yes - I agree with nick.
Flags: needinfo?(mcmanus)
I don't understand how adding telemetry into where the seer 'creates the connection' would be helpful. In my understand the seer doesn't actually create a connection - it calls spec connect which *might* create a connection. Hence why the telemetry needed to be collected in nsHttpConnMgr - this is were the connections are actually created (there a various reasons why it wouldn't create a connection, cheif among them we've already reached our maximum number of speculative connections/connections to that host). I'm fine with keeping the speculative connection telemetry, but I don't see how we can move the seer creation telemetry.

The only time where the seer actually creates a connection is in Prefetch, which as far as the world is concerned doesn't exist yet.
Flags: needinfo?(hurley)
As discussed on irc... yes, you are right about the placement of the telemetry accumulation.
Flags: needinfo?(hurley)
Attachment #8455495 - Flags: review?(hurley) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aca7944ee3df
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.