888628 - Expose half open connections in Networking Dashboard

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Catalin Iordache]

Attachment: halfOpen.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130620123127

Steps to reproduce:

I created another dictionary in NetDashboard.webidl (HalfOpenInfoDict) and also I addead halfOpens array to HttpConnDict dictionary. Also, I added HalfOpenSockets structure in DashboardTypes and halfOpens array in HttpRetParams structure.
With all of this and some more code in nsHttpConnectionMgr.cpp and Dashboard.cpp, we are now able to expose all of half open connections, in Networking Dashboard

Comment 1

[Valentin Gosu [:valentin]]

Comment on attachment 769380 [details] [diff] [review]
halfOpen.patch

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

::: dom/webidl/NetDashboard.webidl
@@ +16,5 @@
>  
>  dictionary HttpConnInfoDict {
>  	sequence<unsigned long> rtt;
>  	sequence<unsigned long> ttl;
>    sequence<octet> spdyVersion;

SpdyVersion should not be in your patch :)

::: netwerk/base/src/Dashboard.cpp
@@ +142,2 @@
>  
>      using mozilla::dom::HttpConnInfoDict;

put using mozilla::dom::HalfOpenInfoDict here, and just user HalfOpenInfoDict in the rest of the method.

@@ +203,5 @@
>              *idle_ttl.AppendElement() = mHttp.data[i].idle[j].ttl;
>          }
> +
> +        mozilla::dom::HalfOpenInfoDict &allHalfOpens = *halfOpens.AppendElement();
> +        allHalfOpens.mSpeculative.Construct();

After this, call SetCapacity. As you can see above.

@@ +204,5 @@
>          }
> +
> +        mozilla::dom::HalfOpenInfoDict &allHalfOpens = *halfOpens.AppendElement();
> +        allHalfOpens.mSpeculative.Construct();
> +        allHalfOpens.mSpeculative.Value();

Save the value. After this you should iterate through mHttp.data[i].halfOpens and append values to mSpeculative.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +3322,5 @@
>          info.rtt = ent->mIdleConns[i]->Rtt();
>          data.idle.AppendElement(info);
>      }
> +    for(uint32_t i = 0; i < ent->mHalfOpens.Length(); i++) {
> +        HalfOpenSockets isSpeculative;

I would give the variable another name. Maybe hSocket?

Comment 2

[Catalin Iordache]

Attachment: Made changes that were specified in review

Comment 3

[Valentin Gosu [:valentin]]

Comment on attachment 769633 [details] [diff] [review]
Made changes that were specified in review

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

Looks good.

Comment 4

[Patrick McManus [:mcmanus]]

I'm happy to have valentin review this patch.

Catalin, please make sure to get a review or delegation of the review from a owner/peer before setting checkin-needed. Its our process.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> I'm happy to have valentin review this patch.
> 
> Catalin, please make sure to get a review or delegation of the review from a
> owner/peer before setting checkin-needed. Its our process.

Also recommended to take checkin-needed off the bug after saying such things ;)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Or is this OK after all? Also, please fix the flags on the patch.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 769633 [details] [diff] [review]
Made changes that were specified in review

this is ready to go.. it has valentin's r+ and I delegated the review to him

Comment 8

[Catalin Iordache]

I'm a little bit confuse. It is all fine? 
Patric, I thought that because Valentin gave me a r+, it is all alright and the patch was needed a check in. I'm sorry if caused you trouble. 
How should I ask for a delegation of the review from the owner/peer ?

Comment 9

[Patrick McManus [:mcmanus]]

Catalin - no worries, I love the contributions! project management is confusing.

To check something in it needs an r+ from an owner or peer or their delegate. These are the necko ones:
https://wiki.mozilla.org/Modules/All#Necko

the owner/peer can, at their discretion, delegate the review to anybody else they think has the background to do the right job. They might do that before or after the patch is posted - depending on how comfortable they are with what is being touched.

For my part, I'm totally fine with routinely delegating dashboard specific code to valentin (as I did here when I wrote "I'm happy to have valentin review this patch") but where that code touches the other parts of necko (for instance to gather more information as robert is doing with http version levels which aren't currently tracked at the connection level) I like to look at it myself. 

One thing you can do is to r?mcmanus AND r?valentin.gosu on your patch and just write "this dashboard code is a candidate for delegation to valentin".. that way the two things can proceed in parallel.

Comment 10

[Catalin Iordache]

Patrick - Thank you for all the explanations.

Comment 11

[Catalin Iordache]

Attachment: halfOpensV3.patch

Rebased the patch against the latest revision of mozilla-centrol; review not needed

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc16f93bf257

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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