assorted fixes and performance improvements #134

sleepybishop · 2020-08-15T01:21:23Z

hello, this PR includes a smattering of fixes for small issues i encountered while using the library, they seem to be common after reviewing the open issues list.

the latest commit improves performance by ~2.5x when compiled with -O3 -march=native -funroll-loops

please review and merge if you find them useful, each fix is in its own branch if some fixes look more palatable than others.

e00E · 2020-08-19T06:18:15Z

src/celt_lpc.c

@@ -96,7 +96,7 @@ void celt_fir(
         int ord)
 {
   int i,j;
-   opus_val16 rnum[ord];
+   opus_val16 *rnum = calloc(sizeof(opus_val16), ord);


Why change VLAs to calloc? VLAs are allocated on the stack and calloc allocates on the heap. Usually people prefer using the stack instead of the heap. The other difference is that calloc 0 initializes the memory but this doesn't seem required in these cases.

agreed, however after browsing the issues list it seemed some folks had trouble compiling under MSVC due to VLA's. calloc was opted for safety.

It's good to support different compilers but I'd prefer some kind of ifdef to fallback to heap allocation only on platforms that don't support VLAs like MSCV because supporting them should not negatively affect more modern compilers.
Or maybe the api of these functions could be changed so that the extra buffer is passed in from the outside? I'm worried about allocating too often because it's unclear to me how how often these functions that use VLAs are called (how hot they are).
Changing the api is probably a bigger change than what you want to accomplish in this PR. Maybe something for a maintainer of the repo to decide.

Looking at the code more I am confused. celt_fir and celt_iir are unused functions. They are never called and the project still compiles when I remove them.
That does still leave the other calloc uses though.

i can revert the VLA related changes if it requires more consideration. i was merely trying to bundle common fixes.

It's totally up to you or a maintainer. I'm not an official maintainer, just voicing my opinion on the PR. Also thanks for bundling the fixes.

You may want to look into libspeex and how it handles VLA support (look out for stack_alloc.h).

i reverted the VLA related changes. thanks for the feedback wrt libspeex, i might try folding this code back into libspeexdsp to replace it's VAD as an experiment, in which case i would definitely adopt existing conventions.

e00E · 2020-08-19T06:22:02Z

src/celt_lpc.c

-   int i, k;
+   int i = 0, k = 0;


I don't doubt that the static analyzer complains but these initializations are not required because either way both variables are set to 0 when they are used. There is a general argument for always obviously initializing everything but this project seems to prefer to initialize only when needed so I feel that this change is inconsistent with the project style.

i think a compromise between initializing everything and being as terse as possible is initializing when a static analyzer highlights a potential problem due to a series of branches, however unlikely that may maybe.

I do believe that these are always initialized (and cannot possibly be unintialized), the static analyzer is just not good enough to figure it out. On the other hand initializing has no drawback either except the mentioned style differences and it is safer in terms of human error. Probably another thing for a maintainer to decide.

e00E · 2020-08-19T06:29:50Z

src/denoise.c

@@ -277,6 +277,7 @@ DenoiseState *rnnoise_create(RNNModel *model) {
 }

 void rnnoise_destroy(DenoiseState *st) {
+  opus_fft_free(common.kfft, 0);


Not sure I understand the surrounding code correctly but I think this is only allocated on demand in check_init so it should only be deallocated if it has been initialized.

fair enough. committed change to only free if common.init == 1

j-schultz · 2020-09-09T09:54:17Z

src/rnn.c

@@ -76,33 +76,39 @@ static OPUS_INLINE float relu(float x)
   return x < 0 ? 0 : x;
 }

+void faxpy(float *restrict a, const rnn_weight *restrict b, int k, float u)


Since this function is not used outside of this translation unit:

Suggested change

void faxpy(float *restrict a, const rnn_weight *restrict b, int k, float u)

static void faxpy(float *restrict a, const rnn_weight *restrict b, int k, float u)

…amard product

petterreinholdtsen · 2021-01-22T20:46:00Z

src/rnn.h

@@ -66,4 +66,4 @@ void compute_gru(const GRULayer *gru, float *state, const float *input);

 void compute_rnn(RNNState *rnn, float *gains, float *vad, const float *input);

-#endif /* _MLP_H_ */
+#endif /* _RNN_H_ */


I am not able to evaluate the rest of the changes, but applied this one to get it out of the way.

sleepybishop changed the title ~~assorted fixes~~ assorted fixes and performance improvements Aug 16, 2020

e00E reviewed Aug 19, 2020

View reviewed changes

sleepybishop added 5 commits August 29, 2020 19:56

free memory still allocated in common struct when finished

a1a1355

fix static analyzer warnings found by clang analyzer

73a482b

define M_PI if it's not, eg. if compiling with -std=c99

2a55474

fix cosmetic typo in rnn.h

de03f80

remove custom asserts and default to ACTIVATION_RELU

4a34847

sleepybishop force-pushed the with_fixes branch from 7c3e90c to d4c1c30 Compare August 30, 2020 02:57

j-schultz reviewed Sep 9, 2020

View reviewed changes

improve perfomance by using blas-like ops (eg. faxpy/fma) for gru had…

accadd3

…amard product

sleepybishop force-pushed the with_fixes branch from d4c1c30 to accadd3 Compare September 9, 2020 16:55

petterreinholdtsen reviewed Jan 22, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assorted fixes and performance improvements #134

assorted fixes and performance improvements #134

sleepybishop commented Aug 15, 2020 •

edited

e00E Aug 19, 2020

sleepybishop Aug 19, 2020

e00E Aug 19, 2020 •

edited

e00E Aug 19, 2020 •

edited

sleepybishop Aug 19, 2020

e00E Aug 19, 2020

j-schultz Sep 9, 2020 •

edited

sleepybishop Sep 9, 2020

e00E Aug 19, 2020 •

edited

sleepybishop Aug 19, 2020

e00E Aug 19, 2020 •

edited

e00E Aug 19, 2020

sleepybishop Aug 19, 2020 •

edited

j-schultz Sep 9, 2020

petterreinholdtsen Jan 22, 2021

	void faxpy(float restrict a, const rnn_weight restrict b, int k, float u)
	static void faxpy(float restrict a, const rnn_weight restrict b, int k, float u)

assorted fixes and performance improvements #134

Are you sure you want to change the base?

assorted fixes and performance improvements #134

Conversation

sleepybishop commented Aug 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e00E Aug 19, 2020 • edited

Choose a reason for hiding this comment

e00E Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-schultz Sep 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e00E Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e00E Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepybishop Aug 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleepybishop commented Aug 15, 2020 •

edited

e00E Aug 19, 2020 •

edited

e00E Aug 19, 2020 •

edited

j-schultz Sep 9, 2020 •

edited

e00E Aug 19, 2020 •

edited

e00E Aug 19, 2020 •

edited

sleepybishop Aug 19, 2020 •

edited