[webkit-reviews] review granted: [Bug 223380] Regression(r273542) Degraded ScriptProcessorNode performance/quality for (14.0.3+ and iOS 14.4.1+) : [Attachment 423499] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 223380: Regression(r273542) Degraded ScriptProcessorNode
performance/quality for (14.0.3+ and iOS 14.4.1+)
https://bugs.webkit.org/show_bug.cgi?id=223380

Attachment 423499: Patch

https://bugs.webkit.org/attachment.cgi?id=423499&action=review




--- Comment #4 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 423499
  --> https://bugs.webkit.org/attachment.cgi?id=423499
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423499&action=review

r=me

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:162
>      // Get input and output buffers. We double-buffer both the input and
output sides.
>      unsigned doubleBufferIndex = this->doubleBufferIndex();
>      bool isDoubleBufferIndexGood = doubleBufferIndex < 2 &&
doubleBufferIndex < m_inputBuffers.size() && doubleBufferIndex <
m_outputBuffers.size();
>      ASSERT(isDoubleBufferIndexGood);
>      if (!isDoubleBufferIndexGood)
>	   return;

This code made it hard for me to understand that doubleBufferIndex is always 0
or 1, and is a simple buffer swapper. It made me think that there are some
conditions under which we choose to set doubleBufferIndex to some other value,
and those conditions are just not reachable at this point of this function.
Which is not true. Since we already have a helper function for
doubleBufferIndex(), if we really want to do validation, let's do it in
doubleBufferIndex(). For example, we can change doubleBufferIndex() to return
std::min(m_doubleBufferIndex, 1). Or we can just remove this nonsense because
it is nonsense.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:225
>	       callOnMainThread([this, doubleBufferIndex = m_doubleBufferIndex,
protector = makeRef(*this)] {
> -		   auto locker = holdLock(m_processLock);
> +		   auto locker = holdLock(m_bufferLocks[doubleBufferIndex]);
>		   fireProcessEvent(doubleBufferIndex);

It's not ideal to re-read m_doubleBufferIndex here (and a few lines above).
Better to capture the local variable doubleBufferIndex instead. They'll always
be the same in practice - but capturing the local helps to clarify that we're
definitely holding the same lock. (Also kinda funny that we do all that
nonsense validation of doubleBufferIndex above and then ignore it here!)

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.h:93
>      Vector<RefPtr<AudioBuffer>> m_inputBuffers;
>      Vector<RefPtr<AudioBuffer>> m_outputBuffers;

Let's change these Vectors to optional<std::array<2>>, to clarify their
relationship to the 2 locks and their use in double buffering.