Closed Bug 1185061 Opened 4 years ago Closed 4 years ago

Report dom.ipc.processCount to Telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: vladan, Assigned: robertthyberg, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 file, 1 obsolete file)

The dom.ipc.processCount pref controls whether multi-process Firefox (also known as e10s) uses a single web content process or whether it spawns a new process for every new tab.

The default configuration is dom.ipc.processCount=1 (i.e. only one content process), but some advanced users are testing out Firefox in process-per-tab mode with processCount > 1. We'd like to be able to distinguish between these configurations when studying Firefox performance using Telemetry.


What is E10S? https://wiki.mozilla.org/Electrolysis
What is Telemetry? https://wiki.mozilla.org/Telemetry

Code to modify: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1105
I suggest that it would be better to return this in the environment block by modifying this list:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#82

It's weird that the e10s setting is separate from everything else in TelemetrySession.
(In reply to Benjamin Smedberg  [:bsmedberg] (away until 27-July) from comment #2)
> I suggest that it would be better to return this in the environment block

ok
hi can i work on this bug?
Flags: needinfo?(vdjeric)
(In reply to rthyberg from comment #4)
> hi can i work on this bug?

Certainly
Flags: needinfo?(vdjeric)
So should this be added to the telemetryEnviroment.jsm or telemetrySession.jsm? I added it to default enviroment vairables. How should i test it? This is my first bug
The location shown in comment 2 would be best.

You can test your changes manually by going to about:telemetry and checking its present in the Environment -> settings section.
It should reflect the current value in about:config.

Automated tests can be run using: mach xpcshell-test toolkit/components/telemetry/tests/unit
Attached patch name.patch (obsolete) — Splinter Review
Attachment #8643440 - Flags: review?(vdjeric)
Flags: needinfo?(vdjeric)
I remembered why I wanted to report this pref outside TelemetryEnvironment. I didn't want to start a new Telemetry sub-session as soon as the pref was changed because the pref only takes effect on restart.

Georg or Benjamin, what do you think?
Flags: needinfo?(vdjeric)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(benjamin)
Comment on attachment 8643440 [details] [diff] [review]
name.patch

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

Looks good
Attachment #8643440 - Flags: review?(vdjeric) → review+
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #9)
> I remembered why I wanted to report this pref outside TelemetryEnvironment.
> I didn't want to start a new Telemetry sub-session as soon as the pref was
> changed because the pref only takes effect on restart.
> 
> Georg or Benjamin, what do you think?

I don't think that would be a problem with that pref for now, we wouldn't expect it to change often.

For the more general issue, we can just add e.g. an optional flag to the pref definitions [0] that means "don't start a new subsession when this changes".
Sounds good?

0: https://dxr.mozilla.org/mozilla-central/rev/07befc6f54e743b8be189cd2c14d74cf1bcef1c2/toolkit/components/telemetry/TelemetryEnvironment.jsm#83
Flags: needinfo?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> For the more general issue, we can just add e.g. an optional flag to the
> pref definitions [0] that means "don't start a new subsession when this
> changes".
> Sounds good?

Yes, can you file another bug?
Flags: needinfo?(benjamin)
TelemetryEnviroment -> TelemetryEnviro*N*ment
fixed the spelling this time
Attachment #8643440 - Attachment is obsolete: true
Flags: needinfo?(vdjeric)
Attachment #8645441 - Flags: review?(vdjeric)
Assignee: nobody → robertthyberg
Status: NEW → ASSIGNED
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #12)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > For the more general issue, we can just add e.g. an optional flag to the
> > pref definitions [0] that means "don't start a new subsession when this
> > changes".
> > Sounds good?
> 
> Yes, can you file another bug?

Filed bug 1192811.
Attachment #8645441 - Flags: review?(vdjeric) → review+
Try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15c43f35b764

But the tree is closed
Flags: needinfo?(vdjeric)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65972984187d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.