Stop locking when getting the preferred samplerate from the AudioStream

RESOLVED FIXED in mozilla29

Status

()

defect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

unspecified
mozilla29
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

This function is super high in the profile because there is a high contention on the lock. Best to remove it.
Assignee: nobody → paul
oops, forgot to rebase over your de-virtualization.
Attachment #8340397 - Flags: review?(kinetik)
Attachment #8340392 - Attachment is obsolete: true
Attachment #8340392 - Flags: review?(kinetik)
(forgot to qref)
Attachment #8340450 - Flags: review?(kinetik)
Attachment #8340397 - Attachment is obsolete: true
Attachment #8340397 - Flags: review?(kinetik)
Comment on attachment 8340450 [details] [diff] [review]
Stop locking when getting the preferred samplerate from the AudioStream. r=

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

::: content/media/AudioStream.h
@@ +382,5 @@
>    static cubeb* sCubebContext;
>  
>    // Prefered samplerate, in Hz (characteristic of the
>    // hardware/mixer/platform/API used).
>    static uint32_t sPreferredSampleRate;

Move this above the mutex comment since it no longer applies.
Attachment #8340450 - Flags: review?(kinetik) → review+
https://hg.mozilla.org/mozilla-central/rev/66be4716e86e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 946037
So, this regressed bug 723793. Oops.
Blocks: 723793
I'm going to back this out for now, reopen the bug, and we'll fix it a different way here.

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b206715056

Sorry for missing this in the review. :-/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
So, backing up a step, why are we calling PreferredSampleRate() so often that it matters how fast it is?

Looks like the only two callers are:
- in the constructor for WebAudio's AudioContext, which shouldn't be particularly performance critical
- in MediaStreamGraphImpl::RunThread, inside the processing loop, via IdealAudioRate(), which is probably where the problem stems from

Does the MSG expect the ideal rate to change?  If not, we can hoist the call to IdealRate() out of the loop.  If it does, we can teach the IdealRate() implementation that the result from PreferredSampleRate() won't change and therefore is safe to cache.
Whiteboard: [leave open]
Posted patch bug944707_v0.patch (obsolete) — Splinter Review
Aside from that question, we could try something like this.  Store the value in an Atomic, and only take the lock when necessary to initialize the value.
Attachment #8340450 - Attachment is obsolete: true
Attachment #8344466 - Flags: review?(paul)
FWIW, I don't know if this patch will help, since an atomic shouldn't be much cheaper than an uncontended lock on platforms with decent locks.
Posted patch patchSplinter Review
Maybe this is better?
Attachment #8345422 - Flags: review?(kinetik)
Attachment #8344466 - Attachment is obsolete: true
Attachment #8344466 - Flags: review?(paul)
Comment on attachment 8345422 [details] [diff] [review]
patch

Looks good.

-  // Get the preferred samplerate for this platform, or fallback to something
"sample rate"

+  // Queries the samplerate the hardware/mixer runs at, and stores it.
ditto

+  // Get the aformentionned sample rate. Does not lock.
aforementioned
Attachment #8345422 - Flags: review?(kinetik) → review+
Relanded with the obvious error fixed. (|| instead of && and use the unlocked version of GetCubebContext)

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc103864507b
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/bc103864507b
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla28 → mozilla29
Depends on: 966867
Depends on: 974232
You need to log in before you can comment on or make changes to this bug.