Reduce allocations to improve performance. #3868
Conversation
Allocating all six oscillators separately is inefficient. It is much more efficient to allocate all of them as a single block.
Update to latest
Allocating all six oscillators separately is inefficient. It is much more efficient to allocate all of them as a single block.
Allocating each oscillator separately was degrading performance when using the Organic instrument.
|
The Organic instrument has the same issue with excessive allocations as the TripleOsc instrument did. I'm adding it to this pull request with the same changes. |
| oscs_r[i + 1] ); | ||
| new (&oscs_l[i]) Oscillator( | ||
| &m_osc[i]->m_waveShapeModel, | ||
| &m_osc[i]->m_modulationAlgoModel, |
There was a problem hiding this comment.
You have unnecessary indentation here
|
@softrabbit or @gi0e5b06 (or someone else) can you please review this? |
There was a problem hiding this comment.
I left a couple of suggestions. Generally speaking, this PR adds yet another quirky, slightly obscure way of manual memory management, but I can't think of a better way to do this, so I'd still say we should merge this.
I'd suggest leaving some comments explaining what's done here, at least in deleteNotePluginData. It's not obvious that delete oscLeft deletes the right oscillators too.
| @@ -42,6 +42,10 @@ class IntModel; | |||
| class EXPORT Oscillator | |||
| { | |||
| MM_OPERATORS | |||
| void* operator new( std::size_t, void* p ) | |||
There was a problem hiding this comment.
Why is this override needed? Isn't this the default implementation?
| @@ -233,8 +233,9 @@ void organicInstrument::playNote( NotePlayHandle * _n, | |||
|
|
|||
| if( _n->totalFramesPlayed() == 0 || _n->m_pluginData == NULL ) | |||
| { | |||
| Oscillator * oscs_l[m_numOscillators]; | |||
| Oscillator * oscs_r[m_numOscillators]; | |||
| Oscillator * oscs_all = MM_ALLOC( Oscillator, m_numOscillators*2 ); | |||
There was a problem hiding this comment.
Seeing that Oscillator has overloaded new operators through the MM_OPERATORS macro, we should be able to replace this with
Oscillator* oscs_all = new Oscillator[m_numOscillators * 2]| Oscillator * oscs_r[m_numOscillators]; | ||
| Oscillator * oscs_all = MM_ALLOC( Oscillator, m_numOscillators*2 ); | ||
| Oscillator * oscs_l = oscs_all; | ||
| Oscillator * oscs_r = oscs_all + m_numOscillators; |
There was a problem hiding this comment.
Maybe this alternative is clearer to readers, but I'll leave it up to you
Oscillator * oscs_l = & oscs_all[0];
Oscillator * oscs_r = & oscs_all[m_numOscillators];| static_cast<oscPtr *>( _n->m_pluginData )->oscLeft = oscs_l[0]; | ||
| static_cast<oscPtr *>( _n->m_pluginData )->oscRight = oscs_r[0]; | ||
| static_cast<oscPtr *>( _n->m_pluginData )->oscLeft = &oscs_l[0]; | ||
| static_cast<oscPtr *>( _n->m_pluginData )->oscRight = &oscs_r[0]; |
There was a problem hiding this comment.
&oscs_l[0] and &oscs_r[0] can be written as oscs_l and oscs_r respectively.
| delete static_cast<Oscillator *>( static_cast<oscPtr *>( | ||
| _n->m_pluginData )->oscRight ); | ||
|
|
||
| MM_FREE( static_cast<oscPtr *>(_n->m_pluginData )->oscLeft); |
There was a problem hiding this comment.
In analogy to the allocation, this should be written as
delete[] static_cast<oscPtr *>(_n->m_pluginData )->oscLeftUsing MM_FREE won't call the object's destructors.
| @@ -300,23 +300,24 @@ void TripleOscillator::playNote( NotePlayHandle * _n, | |||
| { | |||
There was a problem hiding this comment.
Same comments apply to TripleOscillator.cpp.
|
Closing as there's been no reaction to the review and #3873 already mitigates these performance issues greatly. |
This commit partially addresses the poor performance while playing the Skiessi-RandomProjectNumber demo song. This is basically only an issue because of issue #3865, but decreasing the number of allocations is probably good regardless of allocator performance.