It's highly ineffective. Let's consider this benchmark [0], that implements a very classic toy substractive synth. An inverse profiler call tree shows this: 5.1%: nsTArray<AudioTimelineEvent>::AppendElements 2.2%: nsTArray<AudioTimelineEvent>::ReplaceElementsAt 1.4%: nsTArray<AudioTimelineEvent>::~nsTArray Instead, we should: - Keep a range list of period of time where a curve is present, so we can throw synchronously on the main thread (since per spec, we have to throw if authors try to set an event during a curve). We can do other checks without knowing the state of the timeline - Send just the single event to the MSG thread using a ControlMessage - Append this event to the AudioNodeEngine's timeline This should get rid of most nsTArray<AudioTimelineEvent>::* in the profiles. [0]: https://github.com/padenot/webaudio-benchmark/blob/gh-pages/benchmarks.js#L287

(In reply to Paul Adenot (:padenot) from comment #0) > - Keep a range list of period of time where a curve is present, so we can > throw synchronously on the main thread (since per spec, we have to throw if > authors try to set an event during a curve). We can do other checks without > knowing the state of the timeline I was wrong on this one, we need the full timeline because we need to check whether the previous value is zero for an exponential ramp. Either we need to duplicate the timeline, or we need to lock. Locking is not ideal (it's conceptually against the architecture of the MSG), but looks like it would be more efficient. It also potentially has the advantage of lowering the time between the JavaScript call and reflecting it in the msg-side timeline, but I might have other plans to address that. For now I'm going to go with the duplication. Then I'll implement the lock (I think it's easier), and do some testing/benchmarking.

Locks would mean that "immediate" changes would not be processed as a transaction with other timeline changes or other graph changes. IIRC the main thread needs the timeline to for the value getter, which would make duplicating the timeline necessary. I haven't looked at the code, but perhaps more culling of old events can be done.

So in fact, the timeline is already present twice in memory, since we don't clear out the previous data, I didn't know the code enough when I started this, and I had wrong assumption. This allow us to implement `BiquadFilterNode.getFrequencyResponse` properly. We could implement synchronous querying of the value (the `AudioParam.value` getter) in the same way. And indeed culling of old events makes sense even on the main thread.

We stop copying and just send AudioTimelineEvents to the MSG, using the same message-passing mechanism we use for the rest of the main thread -> MSG communication. A couple notable points: - Checking that an event is valid now only occurs on the main thread. When sent, it is considered valid for insertion and is just inserted. This is ok because the two timeline are exact copyies (except MSG's copy pruning, but we only prune behind the AudioContext.currentTime anyways) - We now have to send changes for the `value` attribute of the `AudioParam` and the fact that the input of an `AudioParam` is a node through the same `AudioTimelineEvent`. We take advantage of the fact that there cannot be both a curve and a stream in the same event, so we can safely use an union here. - Conversion between time in seconds (main thread) and time in MSG ticks (MSG thread) happens right before the insertion in the MSG `AudioParamTimeline`. We now use exclusively the getter and not the union, so it's safer. Because we send events and not the full timeline each time we don't have to convert all the events each time.

This is all the (mechanical) changes to convert the nodes to the new way of sending timeline events to the MSG.

Comment on attachment 8659124 [details] [diff] [review] Stop copying AudioParam timelines. r= The approach looks good thanks. A few little things. >+ MediaStream* mStream; Please document what ensures that mStream remains alive, or keep a reference. > private: > static bool IsValid(double value) > { > return mozilla::IsFinite(value); > } >+private: >+ // This member is accessed using the `Time` method, for safety. Gecko convention is to use "private:" only as necessary (once), rather than for each member. >- void InsertEvent(const AudioTimelineEvent& aEvent, ErrorResult& aRv) >+ bool ValidateEvent(AudioTimelineEvent& aEvent, >+ ErrorResult& aRv) > { >+ MOZ_ASSERT(NS_IsMainThread()); >+ >+ if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) || >+ !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) { >+ aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >+ return false; >+ } >+ > if (!aEvent.IsValid()) { > aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >- return; >+ return false; Is there a distinction between timeline-independent checks in ValidateEvent() and those in IsValid()? Could these all be on an AudioTimelineEvent method? >+ const AudioTimelineEvent* GetPreviousEvent(double aTime) const Can this remain private? >-private: >+protected: > // This is a sorted array of the events in the timeline. Queries of this Please remove the "protected:" line to leave this private. There is no need to export these to derived classes AFAICS. >+++ b/dom/media/webaudio/AudioNode.h >+#include "AudioEventTimeline.h" > > namespace mozilla { > > namespace dom { > > class AudioContext; > class AudioBufferSourceNode; > class AudioParam; >@@ -218,18 +219,18 @@ private: >+ static void SendTimelineEventToStream(AudioNode* aNode, uint32_t aIndex, >+ const dom::AudioTimelineEvent& aEvent); Forward declare instead of including in headers, where possible. Otherwise includes get out of control. Here, use "struct AudioTimelineEvent" like the AudioParam declaration. Similarly in AudioNodeEngine.h, AudioNodeStream.h. >- virtual void SetTimelineParameter(uint32_t aIndex, >- const dom::AudioParamTimeline& aValue, >- TrackRate aSampleRate) >+ virtual void SendTimelineEvent(uint32_t aIndex, >+ dom::AudioTimelineEvent& aValue) All the comparable AudioNodeEngine and AudioNodeStream methods are called Set*. "SetTimelineEvent" wouldn't make much sense, so I like SendTimelineEvent on the AudioNodeStream, but its not so appropriate on the engine. Can you call this "AudioNodeEngine::RecvTimelineEvent" or similar, please? (Another option is "AddTimelineEvent", which could be used for both stream and engine methods, but "Recv" does more to imply that the engine is assuming ownership and modifying the event through the non-const reference.) >+ // This sends a single event to the timeline, on the MSG thread side. No comma ',' because it is the timeline that is on the MSG thread, not the send. >- // We override SetValueCurveAtTime to convert the Float32Array to the wrapper >- // object. >- void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv) >+ // We override SetValueCurveAtTime to convert the Float32Array to the wrapper >+ // object. >+ void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv) Indentation. Other methods call InsertEvent(), but SetValueCurveAtTime() doesn't. That would be a change in the behavior of the method. Calling InsertEvent seems the appropriate behavior so that the main thread can calculate the intrinsic value. The validate/insert/callback pattern is frequent. A helper method would tidy up a bit and ensure that the same thing happens in each case. SetValue() calls InsertEvent() but CancelScheduledValues() chains up to the base class method. Can CancelScheduledValues follow the pattern of the others and use the helper. Some methods are calling DOMTimeToStreamTime() on the event time. Others are not. I expect the helper can also create the event and do the conversion. >- if (!WebAudioUtils::IsTimeValid(aStartTime)) { >- aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); The error code is changing to NS_ERROR_DOM_SYNTAX_ERR. I assume that's not intentional. Similarly elsewhere. >+ // Remove some events on the main thread copy. >+ AudioEventTimeline::CancelScheduledValues(DOMTimeToStreamTime(aStartTime)); >+ >+ AudioTimelineEvent event(AudioTimelineEvent::Cancel, aStartTime, 0.0f); Missing DOMTimeToStreamTime(). >+ MOZ_ASSERT(!aSource || aSource->SampleRate() == aDest->SampleRate()); >+ aEvent.SetTimeInTicks( Indentation. >- * Converts AudioParamTimeline floating point time values to tick values >- * with respect to a source and a destination AudioNodeStream. >+ * Converts an AudioTimelineEvent point time values to tick values >+ * with respect to a destination AudioNodeStream. Please keep "floating". Either "Converts an AudioTimelineEvent's floating point time values" or "Converts AudioTimelineEvent floating point time values". >+++ b/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp >+#include "MainThreadUtils.h" Why this change?

Comment on attachment 8659127 [details] [diff] [review] Changes in all the AudioNode implementations. r= Please fold this into the other patch because it won't compile without these changes. The best improvement to readability from dividing up the changes would be to move (and make public) InsertEvent() and ValidateEvent() in a separate patch with no other changes. >+++ b/dom/media/webaudio/BiquadFilterNode.h >@@ -5,16 +5,17 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #ifndef BiquadFilterNode_h_ > #define BiquadFilterNode_h_ > > #include "AudioNode.h" > #include "AudioParam.h" > #include "mozilla/dom/BiquadFilterNodeBinding.h" >+#include "AudioEventTimeline.h" Forward declare.

(In reply to Karl Tomlinson (ni?:karlt) from comment #7) > Comment on attachment 8659124 [details] [diff] [review] > Stop copying AudioParam timelines. r= > > The approach looks good thanks. A few little things. > > >+ MediaStream* mStream; > > Please document what ensures that mStream remains alive, or keep a reference. I've added a strong reference. I also had to mock MediaStream in the unit test. > >- void InsertEvent(const AudioTimelineEvent& aEvent, ErrorResult& aRv) > >+ bool ValidateEvent(AudioTimelineEvent& aEvent, > >+ ErrorResult& aRv) > > { > >+ MOZ_ASSERT(NS_IsMainThread()); > >+ > >+ if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) || > >+ !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) { > >+ aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); > >+ return false; > >+ } > >+ > > if (!aEvent.IsValid()) { > > aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); > >- return; > >+ return false; > > Is there a distinction between timeline-independent checks in ValidateEvent() > and those in IsValid()? Could these all be on an AudioTimelineEvent method? So, we need to return very specific DOM errors depending on certain values in the event. We can't use ErrorResult in AudioTimelineEvent because it's not templated on ErrorResult, and we don't pull in the DOM's ErrorResult in the cppunittest. I've consolidated everything in the ValidateEvent method instead. At least, all the checks are in the sample place now. > >+ const AudioTimelineEvent* GetPreviousEvent(double aTime) const > > Can this remain private? Yes. I'm about to remove it, but I've made it private for now. > >-private: > >+protected: > > // This is a sorted array of the events in the timeline. Queries of this > > Please remove the "protected:" line to leave this private. > There is no need to export these to derived classes AFAICS. Sorry, this was from a previous iteration of the patch. > >- virtual void SetTimelineParameter(uint32_t aIndex, > >- const dom::AudioParamTimeline& aValue, > >- TrackRate aSampleRate) > >+ virtual void SendTimelineEvent(uint32_t aIndex, > >+ dom::AudioTimelineEvent& aValue) > > All the comparable AudioNodeEngine and AudioNodeStream methods are called > Set*. "SetTimelineEvent" wouldn't make much sense, so I like > SendTimelineEvent on the AudioNodeStream, but its not so appropriate on the > engine. Can you call this "AudioNodeEngine::RecvTimelineEvent" or similar, > please? Yes. > >- // We override SetValueCurveAtTime to convert the Float32Array to the wrapper > >- // object. > >- void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv) > >+ // We override SetValueCurveAtTime to convert the Float32Array to the wrapper > >+ // object. > >+ void SetValueCurveAtTime(const Float32Array& aValues, double aStartTime, double aDuration, ErrorResult& aRv) > > Indentation. > > Other methods call InsertEvent(), but SetValueCurveAtTime() doesn't. That > would be a change in the behavior of the method. Calling InsertEvent seems > the appropriate behavior so that the main thread can calculate the intrinsic > value. Yes, that's a mistake. > The validate/insert/callback pattern is frequent. A helper method would tidy > up a bit and ensure that the same thing happens in each case. I've done that. `CancelScheduledValue()` and `SetValue()` should not insert an event on the same thread. Instead, they should modify the timeline on the main thread and then send the event to the MSG thread. > SetValue() calls InsertEvent() but CancelScheduledValues() chains up to the > base > class method. Can CancelScheduledValues follow the pattern of the others > and use the helper. This is a mistake, per above. > Some methods are calling DOMTimeToStreamTime() on the event time. Others are > not. I expect the helper can also create the event and do the conversion. Done. The methods that are creating a special event (SetValue, CancelScheduledValue, Stream), have not been moved to the helper so that the method itself is simple. > > >- if (!WebAudioUtils::IsTimeValid(aStartTime)) { > >- aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); > > The error code is changing to NS_ERROR_DOM_SYNTAX_ERR. > I assume that's not intentional. > Similarly elsewhere. This is all about to change (in bug 1204883). For, I mostly care about the tests passing. > >+++ b/dom/media/webaudio/compiledtest/TestAudioEventTimeline.cpp > > >+#include "MainThreadUtils.h" > > Why this change? The test does not compile anymore otherwise, I've added a number of threading assertion using `NS_IsMainThread()` in `AudioEventTimeline.h` in `AudioTimelineEvent::Time()`.

Here is an interdiff for the first patch.

This is everything folded into one patch.

Comment on attachment 8661231 [details] [diff] [review] Stop copying AudioParam timelines. r= Please don't move GetPreviousEvent(). That is adding noise to the patch and making it harder to review. I haven't yet reviewed the InsertEvent() code moved in this version. >* * * >Bug 1200579 - Changes in all the AudioNode implementations. r= > >This is all the changes to convert the nodes to the new way of sending timeline >event to the MSG. This can be removed. > AudioTimelineEvent(const AudioTimelineEvent& rhs) > { > PodCopy(this, &rhs, 1); > if (rhs.mType == AudioTimelineEvent::SetValueCurve) { > SetCurveParams(rhs.mCurve, rhs.mCurveLength); > } > } > > ~AudioTimelineEvent() > { > if (mType == AudioTimelineEvent::SetValueCurve) { > delete[] mCurve; > } > } These are not dealing with mStream appropriately when mType == Stream. mStream's destructor needs to be run explicitly when appropriate. > template <> > inline double AudioTimelineEvent::Time<double>() const > { > MOZ_ASSERT(!mTimeIsInTicks); >+ MOZ_ASSERT(NS_IsMainThread()); > return mTime; Used off main thread from ConvertAudioTimelineEventToTicks(). >+ // Validate the event itself >+ if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) || >+ !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) { >+ aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >+ return false; >+ } >+ bool timeValueValid = IsValid(aEvent.template Time<double>()) && >+ IsValid(aEvent.mValue) && >+ IsValid(aEvent.mTimeConstant) && >+ IsValid(aEvent.mDuration); WebAudioUtils::IsTimeValid() is stricter than IsValid(), so no need to test Time() and mTimeConstant again. >+++ b/dom/media/webaudio/AudioNodeEngine.h >@@ -5,16 +5,17 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > #ifndef MOZILLA_AUDIONODEENGINE_H_ > #define MOZILLA_AUDIONODEENGINE_H_ > > #include "AudioSegment.h" > #include "mozilla/dom/AudioNode.h" > #include "mozilla/MemoryReporting.h" > #include "mozilla/Mutex.h" >+#include "AudioEventTimeline.h" Forward declare. >+++ b/dom/media/webaudio/AudioParam.h >@@ -10,28 +10,29 @@ > #include "AudioParamTimeline.h" > #include "nsWrapperCache.h" > #include "nsCycleCollectionParticipant.h" > #include "nsAutoPtr.h" > #include "AudioNode.h" > #include "mozilla/dom/TypedArray.h" > #include "WebAudioUtils.h" > #include "js/TypeDecls.h" >+#include "AudioEventTimeline.h" Already included through AudioParamTimeline.h. >@@ -44,87 +45,103 @@ public: > > double DOMTimeToStreamTime(double aTime) const > { > return mNode->Context()->DOMTimeToStreamTime(aTime); > } > > virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override; > >+ void EventInsertionHelper(ErrorResult& aRv, private or protected. (This class is final.) >+ InsertEvent<double>(event); AudioParamTimeline::InsertEvent to skip the unnecessary Cancel/SetValue/Stream handling. (In reply to Paul Adenot (:padenot) from comment #9) > (In reply to Karl Tomlinson (ni?:karlt) from comment #7) > > >+#include "MainThreadUtils.h" > > > > Why this change? > > The test does not compile anymore otherwise, I've added a number of threading > assertion using `NS_IsMainThread()` in `AudioEventTimeline.h` in > `AudioTimelineEvent::Time()`. That should go in AudioTimelineEvent.h then. It's counter-intuitive for callers to need to include headers for their callees.

Comment on attachment 8661231 [details] [diff] [review] Stop copying AudioParam timelines. r= >+ // Remove some events on the main thread copy. >+ AudioEventTimeline::CancelScheduledValues(DOMTimeToStreamTime(aStartTime)); >+ >+ AudioTimelineEvent event(AudioTimelineEvent::Cancel, >+ DOMTimeToStreamTime(aStartTime), 0.0f); DOMTimeToStreamTime() is non-trivial so call the method only once. Either read from the event or use a local variable.

In the copy constructor, need to skip the PodCopy when mType is Stream and use placement new instead of mStream.operator=. http://www.informit.com/guides/content.aspx?g=cplusplus&seqNum=556 may be helpful.

Actually, I guess the PodCopy is ok with placement new after the copy.

Perhaps it is easier manually refcount a MediaStream* AudioTimelineEvent::mStream than using placement new on a nsRefPtr<MediaStream>.

https://hg.mozilla.org/try/rev/9a1c72aad2da has a placement new solution, so that mType is not used uninitialized in the copy constructor, but it seems that the MSVC version doesn't have support for these unions. https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e50d1d9a9e

I think I'll use inheritance to have tighter instances for the `AudioTimelineEvent` and compile everywhere. We can save some bytes, and this is the kind of class that gets instantiated a lot and where each instance is duplicated, on top of that. I've got green tries with patches resembling yours in comment 21, but refactoring this so that it's tighter and looks better is probably not hard so I'll do it before re-requesting review, in a separate patch.

In fact we would need to change a bunch of things that we are changing in bug 1184057, and I expect to get to it soon, so I'll make the event smaller later later, and put mStream out of the union for now. I've taken you patch in comment 21, it's better than what I had.

This has your patch to the copy constructor, and all the comments addressed.

Comment on attachment 8664346 [details] [diff] [review] Stop copying AudioParam timelines. r= (In reply to Paul Adenot (:padenot) from comment #23) > In fact we would need to change a bunch of things that we are changing in > bug 1184057, and I expect to get to it soon, so I'll make the event smaller > later later, and put mStream out of the union for now. Moving it out of the union makes sense for now, but see below for one issue. Inheritance works well for individually heap allocated classes, but AudioTimelineEvent has many stack/member allocations. I think it would be best to avoid allocating each event on the heap individually, and so I suspect a union with manual ref-counting of bare pointers will provide the best long term solution. > ~AudioTimelineEvent() > { > if (mType == AudioTimelineEvent::SetValueCurve) { > delete[] mCurve; >+ } else if (mType == AudioTimelineEvent::Stream) { >+ mStream.~nsRefPtr<MediaStream>(); > } > } Don't add the new code here. Now that mStream is not part of a union, this is not necessary, and I think it will cause problems because mStream's destructor should run after the AudioTimelineEvent destructor. Running the destructor twice will release the reference twice. (In reply to Karl Tomlinson (ni?:karlt) from comment #12) > >* * * > >Bug 1200579 - Changes in all the AudioNode implementations. r= > > > >This is all the changes to convert the nodes to the new way of sending timeline > >event to the MSG. > > This can be removed. To be clear, I wasn't asking for the whole commit message to be removed, just the quoted part left over from the fold. However, I guess much of the comment may be obsolete now anyway. >+ // Validate the event itself >+ if (!WebAudioUtils::IsTimeValid(aEvent.template Time<double>()) || >+ !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) { >+ aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR); >+ return false; >+ } >- if (!WebAudioUtils::IsTimeValid(aStartTime)) { >- aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR); There are two issues with these changes: 1) AudioEventTimeline has these double times after DOMTimeToStreamTime conversion and so they may be negative. This change makes the testcase in bug 864171 throw. I don't like these DOMTimeToStreamTime conversions anyway, so if you are happy with the fix that I'll upload to that bug, then we can ignore this. 2) browser/devtools/webaudioeditor/test/browser_callwatcher-02.js is failing. I assume that is due to these changes. https://treeherder.mozilla.org/#/jobs?repo=try&revision=00e50d1d9a9e For now, I would leave the checks in AudioParam.h and make minimal changes to the checks in AudioEventTimeline.h.