1381627 - Label the EverySecondTelemetryCallback_m runnable

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This runnable happens pretty often. It will need to be labeled. It would be nice if we could use SystemGroup here. But it seems possible that we read data that needs to be read when no other runnables are running.

I also wonder if this runnable could be run at idle priority. That would solve the labeling issue and also improve performance.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Up-ing priority as I learned this is part of the remaining 3% of unlabeled runnables we need to make the concurrent scheduler work effectively for project quantum.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Nico, can you do this one? If not, I can do it.

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Jan-Ivar, sure.

Comment 4

[Andrew Overholt [:overholt]]

Gentle ping here :)

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Thanks for the poke.

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This looks like it labeled in Bug 1377222.
https://hg.mozilla.org/mozilla-central/rev/99566b93d105#l14.14

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Andrew, could you double check me on comment 6 before I close?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

That change just gave it a name. The problem here is the SetTarget call:
http://searchfox.org/mozilla-central/rev/5ce320a16f6998dc2b36e30458131d029d107077/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp#371

We need the target to be the result of calling EventTargetFor on some SchedulerGroup: either the SystemGroup or a DocGroup or TabGroup. If we use the SystemGroup, that means that the timer code is not allowed to modify any state in a way that would be observable by web content. If we use a DocGroup or TabGroup, that means that the timer can modify state in a way that is observable only by the document or tab in question.

So the main issue here is what EverySecondTelemetryCallback_m is actually doing and whether it modifies state in an observable way.

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d846fd0581874efc45293bbae3295de4941ae9

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b7a2c1dba1fbabb438c6fd4cff73545d3c9881

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1381627 - EverySecondTelemetryCallback target

Review commit: https://reviewboard.mozilla.org/r/173842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/173842/

Comment 12

[Andrew Overholt [:overholt]]

Is this landable? :)

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8902310 [details]
Bug 1381627 - EverySecondTelemetryCallback target

https://reviewboard.mozilla.org/r/173842/#review180180

lgtm.

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Andrew Overholt [:overholt] from comment #12)
> Is this landable? :)

Yes, review pending.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8902310 [details]
Bug 1381627 - EverySecondTelemetryCallback target

https://reviewboard.mozilla.org/r/173842/#review180654

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac5161ac285d
EverySecondTelemetryCallback target r=billm,jib

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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