TimeAs and LengthAs slow about drywetmidi HOT 14 CLOSED

Not anywhere as bad on my desktop Win10 machine though... Just under 5 sec overall

However if we compare with MidiSharp-based example (with my patch f3flight/MidiSharp@a7614bd), the results are:

So it's about 200 times faster...
I wonder if we can make DryWetMidi anywhere as fast...

from drywetmidi.

Specifically slow items are these two lines:

• if (!tempoChanges.Any())
• foreach (var tempoChange in tempoChanges.Concat(new[] { new ValueChange<Tempo>(time, tempoLine.AtTime(time)) }))

from drywetmidi.

I made an improvement, let me make a pull request..

from drywetmidi.

with this change I get down to ~1.8s in Release profiling:

without it I get around ~2.8s

from drywetmidi.

it's interesting that after this patch, allocating doubles is slow... whaat?

I'll see if I can improve performance further...

from drywetmidi.

I guess it's just the whole design around IEnumerables which is progressively slower with each added layer of complexity over source data... So getting anywhere near the MidiSharp's result is unlikely without using some sort of cache...

from drywetmidi.

@f3flight Thanks for opening the issue. I am concerned about performance and planning to do some improvements in future releases.

You are right, we need sort of cache to store accumulated microseconds at points of tempo change. I'll think about possible solutions. Maybe I'll introduce something like ITempoMapCache with different implementations for each type of time span and also introduce conversion methods that take that cache instead of TempoMap.

Thanks for pull request, I'll look into it as soon as possible.

from drywetmidi.

I tested caching and performed benchmarks with this code:

foreach (var e in _timedEvents)
{
    var mt = e.TimeAs<MetricTimeSpan>(_tempoMap);
}

Results:
without caching ~3.970 s
with caching ~183.5 ms

So yes, cache gives significant performance boost.

from drywetmidi.

@melanchall sounds great! cant wait to see it implemented, i will then move forward with my project using your lib.

from drywetmidi.

from drywetmidi.

@f3flight I've implemented caching for metric time span conversion (I forgot about linking to an issue inside a commit message so the commit is b008c6c). For this task I've created benchmarks that were ran on this environment:

BenchmarkDotNet=v0.10.12, OS=Windows 10.0.17134
Intel Core i5-7200U CPU 2.50GHz (Kaby Lake), 1 CPU, 4 logical cores and 2 physical cores
Frequency=2648438 Hz, Resolution=377.5810 ns, Timer=TSC
  [Host] : .NET Framework 4.7 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3101.0
  Clr    : .NET Framework 4.7 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3101.0

Results are:

There is no any new public API, caching works internally with regular TempoMap so you don't need to change anything in your code that uses DryWetMIDI.

As you can see from the commit I've created benchmarks for every time span, but note that at the moment performance is improved only for MetricTimeSpan conversion. In fact BarBeatTimeSpan conversion is extremely slow on large collections of events or notes too. It will be handled in another commit, so please don't close the issue.

Please take the latest version of the develop branch and verify that metric conversion is faster now. I'm waiting for your results.

from drywetmidi.

@melanchall I confirm significant (~10x) speed up achieved, thank you very much for this improvement.

from drywetmidi.

The main problem was TempoMap.Tempo.Values and TempoMap.TimeSignature.Values were resorted on every get. So values sorting was performed for each time/length conversion. I eliminated unnecessary sortings and benchmarks results for bar/beat time span conversion are:

@f3flight Can the issue be closed?

from drywetmidi.

I guess so, thank you!

from drywetmidi.