Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster pitch init #172

Closed
wants to merge 3 commits into from
Closed

Conversation

AlexHarker
Copy link
Contributor

@AlexHarker AlexHarker commented Jun 9, 2022

This PR addresses #171.

Essentially it aims not to reallocate memory or recalculate tables where these can be avoided and thus it runs a lot faster in scenarios where BufPitch is being bombarded with small files and thus calling reset() and hence cepstrumF0.init() frequently.

The speed increase for my scenario (in which BufPitch is just one cog) is around 5 times faster, so this cost saving can be really very significant.

Hopefully the changes will be obvious - I also note that there is no mInitialised in DCT which seems a bit odd, but probably this is not 100% consistent across the codebase.

@AlexHarker AlexHarker mentioned this pull request Dec 6, 2022
@weefuzzy
Copy link
Member

weefuzzy commented Dec 6, 2022

I think the changes in this PR happened anyway when I was plugging allocators in

@tremblap
Copy link
Member

@AlexHarker can you see if it is still relevant a PR or close it if not? thanks

@weefuzzy weefuzzy changed the base branch from dev to main February 21, 2023 08:57
@AlexHarker
Copy link
Contributor Author

AlexHarker commented Jun 18, 2023

I believe that some equivalent changes have been made (in that there won't be a reallocation when DCT:init() gets called). However, I suspect that the speed-up here was coming from not reinitialising the DCT tables at all (the trig calculations rather than the memory allocation which is a one-off cost and fairly separate from table size) within CepstrumF0::init() if the DCT table sizes aren't changing - that change in this PR isn't part of the changes that have gone into the main branch.

Thoughts @weefuzzy? Perhaps you don't like this kind of pattern, but it could also be an early exit in DCT::init() which would also potentially benefit other situations. I can do that as a PR (or here) if desirable.

@weefuzzy
Copy link
Member

I think my reason for not spending much time on this before was that I was more inclined to stop doing the DCT this N**2 way altogether, and do the DCT via an FFT instead for logn goodness.

Given that I have no bandwidth for that now, I'm happy enough to improve the existing scheme. Ideally the usage pattern would be similar the FFT objects, but that might be hard.

@AlexHarker
Copy link
Contributor Author

I am closing this as it is superseded by #241 and this will be linked for relevant conversation if needed.

@AlexHarker AlexHarker closed this Aug 16, 2023
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.

3 participants