Refactor lock-protected TelemetryScalar code

RESOLVED FIXED in Firefox 58

Status

()

enhancement
P4
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: mortificador, Mentored)

Tracking

(Blocks 2 bugs)

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

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
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/
Summary: Refactor lock-protected TelemetryHistogram code → Refactor lock-protected TelemetryScalar code
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.
Attachment #8866833 - Flags: review?(gfritzsche)
Working on a rectified patch on the basis of review for bug 1362953, as the bugs are quite similar.
Sorry for the delay - i'm redirecting this review.
Assignee: nobody → vedant.sareen
Attachment #8866833 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
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 :)
Attachment #8866833 - Flags: review?(alessio.placitelli)
Blocks: 1277552
Depends on: 1362953
This bug should be free to work on (per bug 1362953, comment 18).
Assignee: vedantsareen → nobody
Priority: P3 → P4
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.
(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!
Assignee: nobody → mortificador
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!
(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!
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!
Attachment #8918638 - Flags: review?(alessio.placitelli)
(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!
(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 :)
Attachment #8866833 - Attachment is obsolete: true
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!
Attachment #8918638 - Flags: review?(alessio.placitelli) → review+
(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!
(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 :)
Flags: needinfo?(mortificador)
Keywords: checkin-needed
(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! :)
Flags: needinfo?(mortificador)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c830f5c33251
Refactor lock-protected TelemetryScalar code. r=Dexter
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c830f5c33251
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1331575
You need to log in before you can comment on or make changes to this bug.