Bug 113568 - g_slist_reverse() may not be required in webKitWebAudioSrcLoop
Summary: g_slist_reverse() may not be required in webKitWebAudioSrcLoop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Praveen Jadhav
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-29 04:35 PDT by Praveen Jadhav
Modified: 2013-03-30 03:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.26 KB, patch)
2013-03-29 04:43 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2013-03-29 22:49 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff
Patch (2.42 KB, patch)
2013-03-30 02:56 PDT, Praveen Jadhav
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Praveen Jadhav 2013-03-29 04:35:15 PDT
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.
Comment 1 Praveen Jadhav 2013-03-29 04:43:05 PDT
Created attachment 195722 [details]
Patch

Basically, this change reduces the no. of instructions generated for webKitWebAudioSrcLoop.
Comment 2 Philippe Normand 2013-03-29 06:45:33 PDT
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.
Comment 3 Philippe Normand 2013-03-29 07:42:14 PDT
Ok I tested it, if you can improve the ChangeLog a bit it'd be great :)
Comment 4 Praveen Jadhav 2013-03-29 22:43:07 PDT
(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.
Comment 5 Praveen Jadhav 2013-03-29 22:49:25 PDT
Created attachment 195842 [details]
Patch

Patch updated as per review comments.
Comment 6 Praveen Jadhav 2013-03-29 23:22:16 PDT
(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.
Comment 7 Philippe Normand 2013-03-30 01:27:24 PDT
Ok it's good you made some measurements, can you put this explanation in the ChangeLog itself please?
Comment 8 Soo-Hyun Choi 2013-03-30 01:50:10 PDT
FYI - changed assignee to Praveen Jadhav.
Comment 9 Praveen Jadhav 2013-03-30 02:56:23 PDT
Created attachment 195851 [details]
Patch
Comment 10 WebKit Review Bot 2013-03-30 03:39:35 PDT
Comment on attachment 195851 [details]
Patch

Clearing flags on attachment: 195851

Committed r147278: <http://trac.webkit.org/changeset/147278>
Comment 11 WebKit Review Bot 2013-03-30 03:39:39 PDT
All reviewed patches have been landed.  Closing bug.