1362957 - Refactor lock-protected TelemetryScalar code

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Georg Fritzsche [:gfritzsche]]

In TelemetryScalar.cpp [1], all functions that are named `internal_*()` and accessing shared state are meant to be called with a global TelemetryScalar lock to be held (on `gTelemetryScalarsMutex`).

A safer pattern is to require passing a lock instance as seen in [2].
We should refactor TelemetryScalar to that pattern, so e.g.:
> ScalarResult
> internal_CanRecordScalar(...)
... becomes
> ScalarResult
> internal_CanRecordScalar(const StaticMutexAutoLock& lock, ...)
... and callers become:
> internal_CanRecordScalar(locker, ...);

1: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryScalar.cpp
2: https://dxr.mozilla.org/mozilla-central/rev/17d8a1e278a9c54a6fdda9d390abce4077e55b20/toolkit/components/telemetry/TelemetryEvent.cpp#280

Comment 1

[Georg Fritzsche [:gfritzsche]]

This will require:
- Changing TelemetryScalar.cpp as described.
- We will also need to update the comments to match the code changes.
- Making sure tests pass:
  - first: mach test toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
  - before submitting a patch, also make sure the other tests pass too: mach test toolkit/components/telemetry/tests/unit/

Comment 2

[Vedant Sareen [:fionn_mac]]

Attachment: Proposed patch for bug 1362957.

I was able to build successfully. The patch passed all tests.

The methods in the class |KeyedScalar| now also take |const StaticMutexAutoLock& lock| as an argument as they are only called by the |internal_*()| methods and also call some of them, with the lock parameter being passed from function to function in both the cases.

Comment 3

[Vedant Sareen [:fionn_mac]]

Working on a rectified patch on the basis of review for bug 1362953, as the bugs are quite similar.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Sorry for the delay - i'm redirecting this review.

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8866833 [details] [diff] [review]
Proposed patch for bug 1362957.

Let's defer this to when the updated patch is available. I had a quick look at both the patch and bug 1362953: it looks like most feedback from there (new lockers, whitespaces) also applies to this.

Please flag me on the new patch as per comment 3 :)

Comment 6

[Georg Fritzsche [:gfritzsche]]

This bug should be free to work on (per bug 1362953, comment 18).

Comment 7

[Fernando]

Hi,

My name is Fernando, I've never contributed to the Firefox source code before, and I would like to start. I already know C++, and I think this would be a good first bug to work on.

If you think that it's ok, I can start working on a patch.

Thank you very much!

Fernando.

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #7)
> Hi,
> 
> My name is Fernando, I've never contributed to the Firefox source code
> before, and I would like to start. I already know C++, and I think this
> would be a good first bug to work on.

Hi Fernando! I've assigned this bug to you. Please go through the comments above and let me know if anything is unclear!

Comment 9

[Fernando]

Hi!

I'm working on this, but I've spotted https://bugzilla.mozilla.org/show_bug.cgi?id=1362953#c22

"The third will be to rework the TelemetryHistogram code so that TelemetryHistogram::* methods continue to generate locks, passing them as proof to internal_* methods. (see early comments on this bug)"

I'm not sure, but I think that he is describing what needs to be done here.

Is this a duplicate or I did misunderstand something?

Thank you very much!

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #9)
> "The third will be to rework the TelemetryHistogram code so that
> TelemetryHistogram::* methods continue to generate locks, passing them as
> proof to internal_* methods. (see early comments on this bug)"
> 
> I'm not sure, but I think that he is describing what needs to be done here.

This is describing what Georg described in comment 0. Sorry for the confusion!

This bug is not a duplicate of 1362953, but it's related: they are both changing telemetry code in similar ways, but on different files. This is about TelemetryScalar.cpp, the other bug is about TelemetryHistogram.cpp.

Please let me know if you have further questions!

Comment 11

[Fernando]

Hi!

Thank you very much for your answer, I didn't notice that they were talking about another file.

I have a patch for this bug (you can find it attached to this bug report page). I've run the tests:
mach test toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
mach test toolkit/components/telemetry/tests/unit/

And all of them pass.

Should I push my commit for review? Who should I put as reviewer?

Thank you very much!

Comment 12

[Fernando]

Attachment: Proposed patch for bug 1362957

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #11)
> I have a patch for this bug (you can find it attached to this bug report
> page). I've run the tests:
> mach test toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
> mach test toolkit/components/telemetry/tests/unit/
> 
> And all of them pass.
> 
> Should I push my commit for review? Who should I put as reviewer?
> 
> Thank you very much!

Hi Fernando, thank you very much for your work! I've self-assigned myself as a reviewer and I'll get to the review within the next couple of days!

Comment 14

[Fernando]

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> (In reply to Fernando from comment #11)
> > I have a patch for this bug (you can find it attached to this bug report
> > page). I've run the tests:
> > mach test toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
> > mach test toolkit/components/telemetry/tests/unit/
> > 
> > And all of them pass.
> > 
> > Should I push my commit for review? Who should I put as reviewer?
> > 
> > Thank you very much!
> 
> Hi Fernando, thank you very much for your work! I've self-assigned myself as
> a reviewer and I'll get to the review within the next couple of days!

Great! Thank you very much :)

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8918638 [details] [diff] [review]
Proposed patch for bug 1362957

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

Hey Fernando, this looks great, thanks!

Comment 16

[Alessio Placitelli [:Dexter]]

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

Comment 17

[Fernando]

(In reply to Alessio Placitelli [:Dexter] from comment #16)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8ad6d07e03fd7d1895274e04046b11ba6c4662f0

Great, happy to help!

Should I do something once the tests are finished?

Thank you very much!

Comment 18

[Alessio Placitelli [:Dexter]]

(In reply to Fernando from comment #17)
> (In reply to Alessio Placitelli [:Dexter] from comment #16)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=8ad6d07e03fd7d1895274e04046b11ba6c4662f0
> 
> Great, happy to help!
> 
> Should I do something once the tests are finished?

In general, once the tests results are complete, who wrote the patch should check them and make sure that any intermittent failure (orange boxes) or permanent failures (red boxes) are not related to his/her code. However, this requires a mix of experience and knowledge of the codebase so I took care of that for you :-)
If no failures look related to the patch, then the code can be landed: I marked this bug as "checkin-needed" so that, when some other colleague checks in other patches, he/she can take care of this as well (saving resources for batching them together!).

Would you be interested in working on some other C++ or Javascript bug? Do you have any preferences? If so, I could help you find one :)

Comment 19

[Fernando]

(In reply to Alessio Placitelli [:Dexter] from comment #18)
> (In reply to Fernando from comment #17)
> > (In reply to Alessio Placitelli [:Dexter] from comment #16)
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=8ad6d07e03fd7d1895274e04046b11ba6c4662f0
> > 
> > Great, happy to help!
> > 
> > Should I do something once the tests are finished?
> 
> In general, once the tests results are complete, who wrote the patch should
> check them and make sure that any intermittent failure (orange boxes) or
> permanent failures (red boxes) are not related to his/her code. However,
> this requires a mix of experience and knowledge of the codebase so I took
> care of that for you :-)
> If no failures look related to the patch, then the code can be landed: I
> marked this bug as "checkin-needed" so that, when some other colleague
> checks in other patches, he/she can take care of this as well (saving
> resources for batching them together!).
> 
> Would you be interested in working on some other C++ or Javascript bug? Do
> you have any preferences? If so, I could help you find one :)

Cool, thank you very much!

Yes, I'm very interested in working on other bug (preferably C++, my knowledge of Javascript is very limited). I don't have any preference, if you have any suggestion I'd really appreciate it! :)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c830f5c33251
Refactor lock-protected TelemetryScalar code. r=Dexter

Comment 21

[Stefan Hindli [:stefan_hindli]]

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