Skip to content

Conversation

@AlexHarker
Copy link
Contributor

…g the default allocator)

This is an update for #169.

This is not only a speed optimisation. It also fixes the fact that even in SC it will currently allocate an FFT using the default allocator for every frame whoever yinFFT is called.

I will check the tests, which failed last time and make any necessary updates as a second step.

@AlexHarker AlexHarker requested a review from tremblap August 16, 2023 22:58
@AlexHarker
Copy link
Contributor Author

AlexHarker commented Aug 16, 2023

If @weefuzzy has a chance to look at the sizes for instantiation that would be useful [or to say if there's no time for that]

@AlexHarker
Copy link
Contributor Author

Currently broken - and not as relevant as it used to be - investigating...

@AlexHarker
Copy link
Contributor Author

AlexHarker commented Aug 17, 2023

Now fixed.

So:

  • Ths main speed advantage to this approach is no longer relevant as Flucoma now uses a single static FFT setup (creating a setup per frame was previously a major cost but yinFFT is now about twice as fast as it was before - this change was introduced when allocators/memory assignment were dealt with).
  • This PR still addresses lingering issues of memory allocation on the real-time thread, which were not addressed previously
  • It's unclear whether any visible speed benefits still remain

@tremblap tremblap merged commit f916b8e into flucoma:main Aug 17, 2023
@AlexHarker
Copy link
Contributor Author

Did you speed test this? Also did you check if the unit tests work correctly with the two different commits?

@tremblap
Copy link
Member

if it is approved it means it passes my QC. I reproduce expected results and it is faster.

@AlexHarker
Copy link
Contributor Author

Great - I couldn't judge the speed on realtime, but I haven't tried it offline yet - I will do so.

@AlexHarker AlexHarker deleted the faster-yinFFT-rebuild branch January 19, 2024 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants