detectCycles performance

[gerardsmyth]

Hi there,

We have a complex application using Tone.js, which in turn uses standardized-audio-context underneath.

During performance testing we’ve noticed that a large amount of time is being spent inside a detectCycles recursive loop. For example, we are seeing that a triggerAttackRelease call on a synth can spend 80% of the processing time inside detectCycles.

Is there anything we can do to improve the performance of this functionality?

If we strip out standardized-audio-context completely the performance seems much improved, but obviously we are then losing the cross-browser standardisation provided.

Are you able to explain a bit more about when the detectCycles functionality is needed, and the browsers/problems it helps with? Can we do anything to reduce the number of times it gets called?

Many thanks for any info you can provide.

Regards,
Gerard
www.dappledark.com

[chrisguttandin]

Hi Gerard, thanks for reporting this.

detectCycles() is used to fix bug number 164. It's present in any browser but Firefox and causes cycles to not get muted correctly. The function is defined here.

It's recursively tracing the graph. This could be very expensive for big graphs. But the function is also very small. It calls a few other functions though which makes me wonder if any of those could be the culprit. Could you try inspecting the performance profile again to check if any of those internal calls takes a disproportionate amount of time?

Currently detectCycles() is called every time connect() or disconnect() gets called on an AudioNode. It's the simplest way to deal with the problem that the graph cannot be stored directly since that would prevent garbage collection. But there is probably a smarter way to do this.

One optimization I could think of from the top of my head would be to not look for any cycles when connecting a source node to something else. This can never cause a cycle since a source node has no input. It's probably also what happens when you call triggerAttackRelease().

[gerardsmyth]

Hi chrisguttandin,

Thank you for your reply.

This screenshot shows part of the performance trace for a triggerAttackRelease call on a Tone PolySynth. (The green detectCycles and (anonymous) blocks get repeated a lot more times as you scroll down).

It seems like it is the detectCycles function itself that is causing the delay, rather than any other functions it calls. The anonymous entries are for the callback function in the Array.map call inside detectCycles.

As you say, we get this pattern on all the calls to connect() - this is just one example. I would imagine that the audio graph is rather large by this point, but I am not aware of any easy ways to count or quantify it.

I’m not sure if this is what you meant, but I have tried altering the connect() function from audio-node-constructor as follows, but it didn’t seem to make a noticeable impact. This does stop a few calls, but no obvious change to the above trace etc.

…
// Bug #164: Only Firefox detects cycles so far.
if (isNewConnectionToAudioParam && this.numberOfInputs > 0) {
    const cycles = detectCycles([this], destination);
…

Do you have a link to the details of bug 164? I tried to search but couldn’t seem to find it, sorry!

Many thanks for your help.

Gerard

[chrisguttandin]

Hi Gerard, thanks for the quick reply.

These are the expectation tests for the bug in Chrome, Edge and Safari. I tried my best to tag every occurrence in the codebase with the same number.

After taking a closer look at what's going on in Tone's triggerRelease() I believe checking for output-only nodes only solves half of the problem. Every synth seems to create a chain with a GainNode to model the envelope and that GainNode would not be effected by this quick fix.

The snippet that you included above handles connections to an AudioParam. If you want to test if the proposed fix has any effect I would assume it needs to be applied here instead.

Could you please also share the very bottom of the flame graph in the picture above. So far it looks like detectCycle() takes almost no time to run itself since every subsequent block seems to be of the same size.

[gerardsmyth]

Hi chrisguttandin,

Thanks for your message and the links to the expectation tests.

I agree that each instance of detectCycles doesn’t seem to take very long, but I think in this case it is getting called so many times that it adds up! I tried to add a console.count() to detectCycles, and it looks like it is getting called over 7000 times for this one triggerAttackRelease() call.

Its hard to capture the whole graph as it is quite tall, but I have tried to stitch it together:

Zooming in a bit on the very bottom right shows this:

My apologies that I wasn’t clear above, but I did add the this.numberOfInputs > 0 check to both calls to detectCycles from inside connect(). It seemed like the AudioParam part didn’t stop any calls, but the AudioNode part did, but just not enough to make any noticeable difference overall. I’m guessing that this is due to the use of GainNodes that you mention?

As I said before, we are seeing this detectCycles pattern in various places, with this triggerAttackRelease call being just one example. So if there is anything that can be done to improve performance it would be great if it isn’t specific to just this. :)

Thanks again for all your help,
Gerard