Bug 264488 - Use SIMD in HasConstantValue() in BiquadDSPKernel
Summary: Use SIMD in HasConstantValue() in BiquadDSPKernel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-11-09 07:46 PST by Ho Cheung
Modified: 2023-11-13 14:00 PST (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ho Cheung 2023-11-09 07:46:48 PST
Use SIMD to optimize this. This would speed up processing by a factor of 4 because we can process 4 floats at a time.
Comment 1 Chris Dumez 2023-11-09 08:07:29 PST
Pull request: https://github.com/WebKit/WebKit/pull/20233
Comment 2 Ho Cheung 2023-11-09 08:14:57 PST
(In reply to Chris Dumez from comment #1)
> Pull request: https://github.com/WebKit/WebKit/pull/20233

Thank you for taking my patch from Blink. I originally wanted to implement it in Webkit myself, but you have done it for me, so I won't submit a new PR on this issue for the time being lol
Comment 3 Chris Dumez 2023-11-09 08:17:09 PST
(In reply to Ho Cheung from comment #2)
> (In reply to Chris Dumez from comment #1)
> > Pull request: https://github.com/WebKit/WebKit/pull/20233
> 
> Thank you for taking my patch from Blink. I originally wanted to implement
> it in Webkit myself, but you have done it for me, so I won't submit a new PR
> on this issue for the time being lol

Sorry, I realized too late you were the patch author on Blink side. It wasn't clear when you filed the bug that you intended on actually making the code change yourself.

If you end up making more contributions to WebKit (and I hope you do), I encourage you to make it clearer in the bug description that a patch is incoming.
Comment 4 Ho Cheung 2023-11-09 08:21:39 PST
(In reply to Chris Dumez from comment #3)
> (In reply to Ho Cheung from comment #2)
> > (In reply to Chris Dumez from comment #1)
> > > Pull request: https://github.com/WebKit/WebKit/pull/20233
> > 
> > Thank you for taking my patch from Blink. I originally wanted to implement
> > it in Webkit myself, but you have done it for me, so I won't submit a new PR
> > on this issue for the time being lol
> 
> Sorry, I realized too late you were the patch author on Blink side. It
> wasn't clear when you filed the bug that you intended on actually making the
> code change yourself.
> 
> If you end up making more contributions to WebKit (and I hope you do), I
> encourage you to make it clearer in the bug description that a patch is
> incoming.

This is my first time submitting a bug report to WebKit. Thank you for your suggestions and help.

Another question I would like to ask is that on the Blink side, I wrote a unit test for this modification. Does this also apply to WebKit?

https://chromium-review.googlesource.com/c/chromium/src/+/4622707
Comment 5 Chris Dumez 2023-11-09 08:31:33 PST
(In reply to Ho Cheung from comment #4)
> (In reply to Chris Dumez from comment #3)
> > (In reply to Ho Cheung from comment #2)
> > > (In reply to Chris Dumez from comment #1)
> > > > Pull request: https://github.com/WebKit/WebKit/pull/20233
> > > 
> > > Thank you for taking my patch from Blink. I originally wanted to implement
> > > it in Webkit myself, but you have done it for me, so I won't submit a new PR
> > > on this issue for the time being lol
> > 
> > Sorry, I realized too late you were the patch author on Blink side. It
> > wasn't clear when you filed the bug that you intended on actually making the
> > code change yourself.
> > 
> > If you end up making more contributions to WebKit (and I hope you do), I
> > encourage you to make it clearer in the bug description that a patch is
> > incoming.
> 
> This is my first time submitting a bug report to WebKit. Thank you for your
> suggestions and help.
> 
> Another question I would like to ask is that on the Blink side, I wrote a
> unit test for this modification. Does this also apply to WebKit?
> 
> https://chromium-review.googlesource.com/c/chromium/src/+/4622707

Oh, I missed that. In general, yes but API tests are located in a different place in WebKit:
- Tools/TestWebKitAPI/Tests/

or Tools/TestWebKitAPI/Tests/WebCore in this case.

I'll look into updating my PR to include your test soon.
Comment 6 EWS 2023-11-13 13:59:54 PST
Committed 270669@main (0a313d9769ba): <https://commits.webkit.org/270669@main>

Reviewed commits have been landed. Closing PR #20233 and removing active labels.
Comment 7 Radar WebKit Bug Importer 2023-11-13 14:00:16 PST
<rdar://problem/118354762>