Audio: Optimize sink checks in crossover_s32_default by ShriramShastry · Pull Request #9178 · thesofproject/sof

ShriramShastry · 2024-05-31T05:43:42Z

Optimize crossover_s32_default by pre-determining the active sinks before processing frames. This check-in reduces redundant null checks for each frame and channel, leading to performance improvements in processing audio streams with multiple sinks.

lyakh

ok, this optimisation avoids checking inactive sinks for each frame on each channel. So yes, it should save some time when there are inactive sinks. But please restore the original index calculation

lyakh · 2024-05-31T07:17:22Z

src/audio/crossover/crossover_generic.c

-			}
-
-			idx += nch;
+				}


indentation

lyakh · 2024-05-31T07:21:46Z

src/audio/crossover/crossover_generic.c

 		for (i = 0; i < frames; i++) {
-			x = audio_stream_read_frag_s32(source_stream, idx);
+			/* Read source */
+			x = audio_stream_read_frag_s32(source_stream, ch + i * nch);


you replaced an idx += nch addition with an addition and a multiplication ch + i * nch - subject to any compiler optimisations. Depending on those optimisations this might or might not end up generating different code, but I personally don't think this would be faster. I'd keep the original addition, personally, I prefer this form:

for (i = 0, idx = ch; i < frames; i++, idx += nch) {

but that might be a matter of taste. The original code was run-time equivalent to this, don't think your version is an improvement.

lyakh · 2024-05-31T07:22:03Z

src/audio/crossover/crossover_generic.c

-			x = audio_stream_read_frag_s32(source_stream, idx);
+			/* Read source */
+			x = audio_stream_read_frag_s32(source_stream, ch + i * nch);
+			/* Apply crossover split */


does this comment add any information?

lyakh · 2024-05-31T07:22:55Z

src/audio/crossover/crossover_generic.c

-								idx);
+			/* Write output to active sinks */
+			for (j = 0; j < active_sinks; j++) {
+				y = audio_stream_write_frag_s32(sink_stream[j], ch + i * nch);


and now you have this calculation twice? No, please, remove this.

ShriramShastry

Thank you for your code review feedback. I reconsidered the index increment approach and agree that incrementing the index directly within the loop construct is both stylistically clearer and potentially more efficient because it eliminates unnecessary multiplication. Regarding the comment, I recognise that it is redundant and should be removed for clarity. I'll make these changes accordingly.

lyakh · 2024-06-03T06:12:32Z

src/audio/crossover/crossover_generic.c

-		for (i = 0; i < frames; i++) {
+		/* Iterate over frames */
+		for (i = 0, idx = ch; i < frames; i++, idx += nch) {
+			 /* Read source */


indentation - there's a spurious space in front of the comment

Done, Thanks.

issue addressed, thanks, would be nice to clean up indentation though

kv2019i

A few minor style comments inline, but the optimization itself seems good and gets and check out of the inner loop.

src/audio/crossover/crossover_generic.c

lgirdwood

Best to split this PR in patches that each have one change. Easiest way to do this quickly is to git reset on to origin/main HEAD (this will leave all your changes as a diff and not as patches), then use git gui to individually pick lines for each patch.

lgirdwood · 2024-06-25T15:49:27Z

src/audio/crossover/crossover_generic.c

+	for (j = 0; j < num_sinks; j++) {
+		if (bsinks[j])
+			sink_stream[active_sinks++] = bsinks[j]->data;
+	}
+
+	/* Process for each channel */
 	for (ch = 0; ch < nch; ch++) {
-		idx = ch;
+		/* Set current crossover state */
 		state = &cd->state[ch];
-		for (i = 0; i < frames; i++) {
+		/* Iterate over frames */
+		for (i = 0, idx = ch; i < frames; i++, idx += nch) {
+			/* Read source */
 			x = audio_stream_read_frag_s32(source_stream, idx);
 			cd->crossover_split(*x, out, state);
-
-			for (j = 0; j < num_sinks; j++) {
-				if (!bsinks[j])
-					continue;
-				sink_stream = bsinks[j]->data;
-				y = audio_stream_write_frag_s32(sink_stream,
-								idx);
+			/* Write output to active sinks */
+			for (j = 0; j < active_sinks; j++) {
+				y = audio_stream_write_frag_s32(sink_stream[j], idx);
+				/* Copy processed data to sink */


Best to split this, as this part is more than the commit message. I would make this a second patch that states the reason and any performance improvement data.

src/audio/crossover/crossover_generic.c

cujomalainey · 2024-06-25T18:15:55Z

src/audio/crossover/crossover_generic.c

 				  int32_t num_sinks,
 				  uint32_t frames)
 {
-	/* Array to hold active sink streams; initialized to null */


all this commit does is delete the comments you added, please check your commits to make sure they make sense before pushing them.

cujomalainey

all my previous comments are still valid, please fix them

Optimize the function by pre-initializing active sinks and adjusting loops for better performance and readability. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>

Add Doxygen comments to describe the parameters and functionality of the crossover_s32_default function. This helps in understanding the function's flow and the purpose of each section. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>

ShriramShastry

Thank you for reviewing the change. I addressed the review comments. Please have a look.

src/audio/crossover/crossover_generic.c

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from aeb61cb to f32aebb Compare May 31, 2024 05:47

ShriramShastry requested review from lgirdwood, lyakh and singalsu May 31, 2024 05:50

lyakh previously requested changes May 31, 2024

View reviewed changes

ShriramShastry commented May 31, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from f32aebb to 4e0131a Compare May 31, 2024 12:58

ShriramShastry requested a review from lyakh May 31, 2024 13:18

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from 4e0131a to 66b65fb Compare May 31, 2024 13:29

ShriramShastry marked this pull request as ready for review June 1, 2024 02:20

ShriramShastry requested a review from a team as a code owner June 1, 2024 02:20

lyakh reviewed Jun 3, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from 66b65fb to 451f364 Compare June 3, 2024 07:48

ShriramShastry requested a review from lyakh June 3, 2024 09:32

lyakh approved these changes Jun 4, 2024

View reviewed changes

kv2019i reviewed Jun 4, 2024

View reviewed changes

src/audio/crossover/crossover_generic.c Show resolved Hide resolved

cujomalainey suggested changes Jun 4, 2024

View reviewed changes

src/audio/crossover/crossover_generic.c Show resolved Hide resolved

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from 451f364 to e9c1e3a Compare June 24, 2024 06:33

ShriramShastry requested review from cujomalainey and kv2019i June 24, 2024 06:34

lgirdwood reviewed Jun 25, 2024

View reviewed changes

cujomalainey suggested changes Jun 25, 2024

View reviewed changes

lgirdwood added this to the v2.11 milestone Jun 26, 2024

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from e9c1e3a to a41f27d Compare August 20, 2024 06:18

ShriramShastry requested a review from cujomalainey August 20, 2024 06:56

cujomalainey suggested changes Aug 20, 2024

View reviewed changes

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from a41f27d to 833cfb6 Compare August 24, 2024 13:09

ShriramShastry added 2 commits August 25, 2024 10:44

Audio: Optimize crossover_s32_default function for performance

362d50a

Optimize the function by pre-initializing active sinks and adjusting loops for better performance and readability. Signed-off-by: Shriram Shastry <malladi.sastry@intel.com>

ShriramShastry force-pushed the optimize_crossover_generic_enhancements branch from 833cfb6 to d000d67 Compare August 25, 2024 07:00

ShriramShastry requested a review from cujomalainey August 26, 2024 03:22

ShriramShastry commented Aug 26, 2024

View reviewed changes

src/audio/crossover/crossover_generic.c Show resolved Hide resolved

marcinszkudlinski approved these changes Aug 26, 2024

View reviewed changes

ShriramShastry requested a review from lgirdwood August 26, 2024 09:08

cujomalainey approved these changes Aug 26, 2024

View reviewed changes

cujomalainey merged commit b381f55 into thesofproject:main Aug 26, 2024

Conversation

ShriramShastry commented May 31, 2024

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShriramShastry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

ShriramShastry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants