Expose half open connections in Networking Dashboard

RESOLVED FIXED in mozilla25

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Catalin Iordache, Assigned: Catalin Iordache)

Tracking

unspecified
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 769380 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Attachment #769380 - Flags: review?(valentin.gosu)
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?
Attachment #769380 - Flags: review?(valentin.gosu)
(Assignee)

Comment 2

4 years ago
Created attachment 769633 [details] [diff] [review]
Made changes that were specified in review
Attachment #769380 - Attachment is obsolete: true
Attachment #769633 - Flags: review?(valentin.gosu)
Comment on attachment 769633 [details] [diff] [review]
Made changes that were specified in review

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

Looks good.
Attachment #769633 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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.
(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 ;)
Keywords: checkin-needed
Or is this OK after all? Also, please fix the flags on the patch.
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
Attachment #769633 - Flags: review?(valentin.gosu)
(Assignee)

Comment 8

4 years ago
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 ?
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.
(Assignee)

Comment 10

4 years ago
Patrick - Thank you for all the explanations.
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 11

4 years ago
Created attachment 779771 [details] [diff] [review]
halfOpensV3.patch

Rebased the patch against the latest revision of mozilla-centrol; review not needed
Attachment #769633 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc16f93bf257
Assignee: nobody → catalinn.iordache
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc16f93bf257
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.