-
Notifications
You must be signed in to change notification settings - Fork 1
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
Perf/rhythm seeking #28
Conversation
869558e
to
774fde6
Compare
Benchmark for 03cbd37Click to view benchmark
|
I'm stupid and forgot the discussion we already had about this :) The only state that needs to be managed here is the random state of the cycle, which actually only is needed when the cycle is seeded. All other state is derived from the cycle's iteration count. So this commit can be removed. Instead, |
774fde6
to
322f760
Compare
Benchmark for 6227750Click to view benchmark
|
… to GenericRhythm
… state from the iteration count
…ls completely while seeking
322f760
to
0e3e93b
Compare
Benchmark for 1d73755Click to view benchmark
|
The random output in the cycle currently has a regression in that it is no longer being deterministic based on the seed, since I've added the proper pattern mult and division. Seeding once per iteration will still not be deterministic enough if we want to mirror tidalcycles. Here is a quote from my previous comment on this
I've added the So apart from a top level seed we would need the random generator to be sampled by position instead of just yielding some value when |
The current behaviour is indeed still not correct (not behaving like the tidal reference implementation), but this is somewhat out of scope here. I would make this a follow-on TODO/PR, okay?
With the introduction of And If we're going to change that, it might be easier and more efficient to use a simpler PRNG, where the state index can be set directly, rather than having to reseed it every time. This should be possible with e.g. a simple Mersenne Twister. |
@unlessgames okay for you to close and merge this? Then I can move on from here... |
Sounds good! Maybe it will be best to implement this once the last feature for tidal is complete. |
Speed up rhythm/phrase seeking. See #6
when seeking phrases & rhythms batch process event iteration where possible
try avoiding as much processing as possible while seeking, by using new "omit" event iter functions and cycle processing functions, which only maintain internal event iter state without actually computing events.
This is WIP, and already ~2x faster in many cases, but unfortunately still not fast enough. Ideal would be speedups of ~8x in normal processing vs seeking. When live editing phrase scripts, the seek function is called quite often for quite long periods of time (minutes up to hours). So this is very performance critical.
@unlessgames As mentioned above, I've also added a new "omit" function to Cycle, but the implementation is rather ugly, but maybe still better than duplicating the whole "output" function for seeking. In general, anything that can be skipped should be skipped there - without breaking the internal state across cycle passes. To make this easier to check, I've added some tests for this, and also reorganised the tests to separate the seek tests from the others.
See especially this change here: 774fde6
You may have some better ideas on how to fix or improve this.
PS: Omit is quite a shitty name for this, but unfortunately "skip" clashes with the standard Rust Iterator impls. I'll clean this skip vs seek vs omit wording after this PR is merged. This will change things across the whole repository. This PR already now is too big.
I'll now concentrate on optimising the other parts of the rhythms: generator patterns, emitters.