g_slist_reverse() may not be required in webKitWebAudioSrcLoop

• Component: Web Audio
• Status: RESOLVED FIXED

[Praveen Jadhav]

webKitWebAudioSrcLoop() will be called continuously after AudioContext instance initialization which affects CPU load. Using decrement counter in for loop can reduce no. of instructions generated (when comparison is done with 0 in jump instruction). Also, g_slist_reverse() need not be used in that case.

[Praveen Jadhav]

Created attachment 195722 [details]
Patch

Basically, this change reduces the no. of instructions generated for webKitWebAudioSrcLoop.

[Philippe Normand]

Comment on attachment 195722 [details]
Patch

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

Looks good but I'd like to test this patch before landing

> Source/WebCore/ChangeLog:8
> +        Decremental for loop logic implemented to avoid using g_slist_reverse().

This could be expanded a bit, explaining the performance gain.

> Source/WebCore/ChangeLog:10
> +        No new tests (OOPS!).

You'd need to remove this line unless you state the patch is covered by existing webaudio tests.

[Philippe Normand]

Ok I tested it, if you can improve the ChangeLog a bit it'd be great :)

[Praveen Jadhav]

(In reply to comment #2)
> (From update of attachment 195722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195722&action=review
> 
> Looks good but I'd like to test this patch before landing
> 
> > Source/WebCore/ChangeLog:8
> > +        Decremental for loop logic implemented to avoid using g_slist_reverse().
> 
> This could be expanded a bit, explaining the performance gain.

I tested with basic audiocontext with 2 destination channels only(No other nodes webaudio nodes are created). Calculated the time taken to execute webKitWebAudioSrcLoop() for 100000 iteration and the details are as below.

Original code                        - 2.025230 micro seconds
Original code + patch                - 1.964759 micro seconds

Its a small gain indeed, but a gain nevertheless. Given the frequency with which webKitWebAudioSrcLoop() is called, CPU load should decrease a bit.

> 
> > Source/WebCore/ChangeLog:10
> > +        No new tests (OOPS!).
> 
> You'd need to remove this line unless you state the patch is covered by existing webaudio tests.

I will update the patch shortly.

[Praveen Jadhav]

Created attachment 195842 [details]
Patch

Patch updated as per review comments.

[Praveen Jadhav]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 195722 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=195722&action=review
> > 
> > Looks good but I'd like to test this patch before landing
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Decremental for loop logic implemented to avoid using g_slist_reverse().
> > 
> > This could be expanded a bit, explaining the performance gain.
> 
> I tested with basic audiocontext with 2 destination channels only(No other nodes webaudio nodes are created). Calculated the time taken to execute webKitWebAudioSrcLoop() for 100000 iteration and the details are as below.
> 
> Original code                        - 2.025230 micro seconds
> Original code + patch                - 1.964759 micro seconds

Correction: it is

Original code                        - 2.025230 micro seconds per function execution
Original code + patch                - 1.964759 micro seconds per function execution

> 
> Its a small gain indeed, but a gain nevertheless. Given the frequency with which webKitWebAudioSrcLoop() is called, CPU load should decrease a bit.
> 
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        No new tests (OOPS!).
> > 
> > You'd need to remove this line unless you state the patch is covered by existing webaudio tests.
> 
> I will update the patch shortly.

[Philippe Normand]

Ok it's good you made some measurements, can you put this explanation in the ChangeLog itself please?

[Soo-Hyun Choi]

FYI - changed assignee to Praveen Jadhav.

[Praveen Jadhav]

Created attachment 195851 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195851 [details]
Patch

Clearing flags on attachment: 195851

Committed r147278: <http://trac.webkit.org/changeset/147278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.