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)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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/
(Reporter)

Updated

2 years ago
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.
(Reporter)

Comment 4

2 years ago
Sorry for the delay - i'm redirecting this review.
Assignee: nobody → vedant.sareen
(Reporter)

Updated

2 years ago
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)
(Reporter)

Updated

2 years ago
Blocks: 1277552
(Reporter)

Updated

2 years ago
Depends on: 1362953
(Reporter)

Comment 6

2 years ago
This bug should be free to work on (per bug 1362953, comment 18).
Assignee: vedantsareen → nobody
Priority: P3 → P4
(Assignee)

Comment 7

2 years ago
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
(Assignee)

Comment 9

2 years ago
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!
(Assignee)

Comment 11

2 years ago
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!
(Assignee)

Comment 12

2 years ago
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!
(Assignee)

Comment 14

2 years ago
(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+
(Assignee)

Comment 17

2 years ago
(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
(Assignee)

Comment 19

2 years ago
(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)

Comment 20

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c830f5c33251
Refactor lock-protected TelemetryScalar code. r=Dexter
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c830f5c33251
Status: NEW → RESOLVED
Last Resolved: 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.