1024639 - Telemetry for seer-created connection use

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[u408661]

It would be good to get some stats on how often connections created by the seer are actually used in practice, so we can adjust the algorithm if/as necessary.

Jeremy, does this seem like something you'd be able to take on this summer?

Comment 1

[Jeremy Poulin [:jaypoulz]]

Absolutely - I can work on this when I'm waiting for feedback on my other work.

Comment 2

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1024639-fix.patch

https://tbpl.mozilla.org/?tree=Try&rev=e488d1c49ba8

I tried modeling it off the telemetry in Predictor.cpp, but I was getting a 'requires dynamic R_X86_64_PC32 reloc against VARIABLE_NAME which may overflow at runtime; recompile with -fPIC' linker error.  I presume this has something to do what how the libraries are actually compiled. This build, which has the variable declarations inline, had no such issues.

Comment 3

[u408661]

Comment on attachment 8441032 [details] [diff] [review]
bug-1024639-fix.patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1375,5 @@
>                   "Found a speculative half open connection\n",
>                   ent->mConnInfo->HashKey().get()));
>              ent->mHalfOpens[i]->SetSpeculative(false);
> +            Telemetry::AutoCounter<Telemetry::HTTPCONNMGR_USED_SPECULATIVE_CONN> mUsedSpeculativeConn;
> +            ++mUsedSpeculativeConn;

So I don't think this will do what we want. Looking at the implementation of AutoCounter, it doesn't read the existing value, and so you'll just keep accumulating 1 for all these telemetry probes... definitely not what we need!

From your comment when uploading this patch, it sounds like you originally tried doing this the right-ish way (keeping a count and only accumulating when everything's wrapping up), but ran up against the compiler doing what compilers do best (angering developers over worthless semantic crap).

Go ahead and upload the old, non-compiling version of the patch, and we can try to figure out what the compiler was complaining about so we can get the data we want :)

Comment 4

[Jeremy Poulin [:jaypoulz]]

Hold on - if auto-counters don't persist, then how is telemetry data gathered across sessions?

Comment 5

[Jeremy Poulin [:jaypoulz]]

Also - I tested this, and it counted up to 3.

Comment 6

[Jeremy Poulin [:jaypoulz]]

(In reply to Jeremy Poulin [:jerzilla] from comment #5)
> Also - I tested this, and it counted up to 3.

I should probaby elaborate and say that I assume it would count higher, but I had disabled seer, so I had no way of reliably generating speculative connections.

Comment 7

[u408661]

Comment on attachment 8441032 [details] [diff] [review]
bug-1024639-fix.patch

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

Huh, so it appears I don't understand how AutoCounters work. Not really surprising, though I wish I'd known sooner...

Just a couple minor fixups, no need to do another try run.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1375,5 @@
>                   "Found a speculative half open connection\n",
>                   ent->mConnInfo->HashKey().get()));
>              ent->mHalfOpens[i]->SetSpeculative(false);
> +            Telemetry::AutoCounter<Telemetry::HTTPCONNMGR_USED_SPECULATIVE_CONN> mUsedSpeculativeConn;
> +            ++mUsedSpeculativeConn;

Rename this usedSpeculativeConnections

@@ +2036,3 @@
>          sock->SetSpeculative(true);
> +        Telemetry::AutoCounter<Telemetry::HTTPCONNMGR_TOTAL_SPECULATIVE_CONN> mTotalSpeculativeConn;
> +        ++mTotalSpeculativeConn;

Rename this totalSpeculativeConnections

@@ +3012,5 @@
>  nsHttpConnectionMgr::nsHalfOpenSocket::Abandon()
>  {
> +    if (this->IsSpeculative()) {
> +      Telemetry::AutoCounter<Telemetry::HTTPCONNMGR_UNUSED_SPECULATIVE_CONN> mUnusedSpeculativeConn;
> +      ++mUnusedSpeculativeConn;

Rename this unusedSpeculativeConnections

Comment 8

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1024639-fix-v2.patch

Here is the broken version I was talking about. If this is the style you want, I'll fold the diffs together. For now this diff is based on my prev patch.

Comment 9

[u408661]

Comment on attachment 8441608 [details] [diff] [review]
bug-1024639-fix-v2.patch

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

Yeah, based on comment #5 and my apparent misunderstanding of AutoCounters, let's stick with your previous version (with the changes I mentioned in comment #7)

Comment 10

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1024639-fix.patch

Now with consistent naming conventions :)

Comment 11

[u408661]

Comment on attachment 8441615 [details] [diff] [review]
bug-1024639-fix.patch

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

Looks good. Fix up the one bit below, upload a new patch, carry forward r+, and mark it checkin-needed.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3010,5 @@
>  
>  void
>  nsHttpConnectionMgr::nsHalfOpenSocket::Abandon()
>  {
> +    if (this->IsSpeculative()) {

No need for "this->" here

Comment 12

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1024639-fix.patch

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dab457aed98

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2dab457aed98