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

Fix "out of buffers" crash #3783

Merged
merged 9 commits into from Sep 26, 2017
Merged

Fix "out of buffers" crash #3783

merged 9 commits into from Sep 26, 2017

Conversation

@lukas-w
Copy link
Member

@lukas-w lukas-w commented Aug 30, 2017

Fixes #3223

Replaces BufferManager's implementation with simple calls to `MemoryManager::alloc. Re-use buffers where they are allocated (AudioPort.cpp & PlayHandle.cpp). I don't experience any performance regression from this change.

My initial approach to fix #3223 was to re-implement BufferManager using a lock-free stack, see b6421d4. However, I couldn't get Travis to build with it, and I realized BufferManager::acquire is only used in two places that can be optimized easily.

lukas-w added 5 commits Aug 29, 2017
Re-implement BufferManager using a single lock-free stack instead of two
fixed-size arrays. This has the following advantages:
 * Less code
 * Immediate re-use of released buffers, no blocking "refresh" required
 * Dynamic size, no crash when out of buffers (fixes #3670)
Use MemoryManager allocation instead and re-use buffers where they are
allocated (AudioPort.cpp & PlayHandle.cpp)
Boost not used anymore.

This reverts commit 6b557d2.
@lukas-w lukas-w changed the title Remove BufferManager implementation Fix "out of buffers" crash Sep 1, 2017
Copy link
Member

@Umcaruje Umcaruje left a comment

In general, there is a lot of inconsistency with the spaces in the brackets, I vote they all get removed because spaces are not a coding convention anymore.

@@ -31,7 +31,7 @@
class MemoryHelper {
public:

static void* alignedMalloc( int );
static void* alignedMalloc(size_t );

This comment has been minimized.

@Umcaruje

Umcaruje Sep 7, 2017
Member

inconsistent spaces inside the bracket here

m_chunks( chunks )
{
m_free = (char*) MemoryHelper::alignedMalloc( chunks );
m_free = reinterpret_cast<char*>(MemoryHelper::alignedMalloc( chunks ));

This comment has been minimized.

@Umcaruje

Umcaruje Sep 7, 2017
Member

same inconsistency here, chunks should have spaces removed

@tresf tresf added this to the 1.2.0 milestone Sep 11, 2017
int i = s_releasedIndex.fetchAndAddOrdered( 1 );
s_released[ i ] = buf;
//qDebug( "released buffer: %p - index %d", buf, i );
MM_FREE(buf);

This comment has been minimized.

@Umcaruje

Umcaruje Sep 17, 2017
Member

I'm a nitpick, but space missing here too. Thanks for fixing the other lines.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Sep 17, 2017

@Umcaruje I fixed some inconsistencies. However, I'd like to avoid touching any code just for the sake of fixing code format, because it makes reviewing commit history (especially using git blame) a lot harder.

{
MemoryManager::free( p );
}

typedef std::vector<T, MmAllocator<T> > vector;
typedef std::vector<T, MmAllocator<T>> vector;

This comment has been minimized.

@Umcaruje

Umcaruje Sep 17, 2017
Member

looks like the space here was removed again, have an error while compiling.

error: ‘>>’ should be ‘> >’ within a nested template argument list

This comment has been minimized.

@lukas-w

lukas-w Sep 17, 2017
Author Member

Right, should have remembered from 73728d7

Same as 73728d7, accidentally removed
space in aebbe22
@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Sep 17, 2017

The original project from the bug report still crashes for me with this PR. backtrace:

Thread 11 "QThread" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff859a0700 (LWP 7287)]
0x0000000000579116 in QGenericAtomicOps<QBasicAtomicOps<8> >::storeRelease<ThreadableJob*, ThreadableJob*> (newValue=0x7fffe7ff8f48, _q_value=@0xffffffff40998c60: <error reading variable>)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qgenericatomic.h:111
111	        *static_cast<volatile T *>(&_q_value) = newValue;
(gdb) bt full
#0  0x0000000000579116 in QGenericAtomicOps<QBasicAtomicOps<8> >::storeRelease<ThreadableJob*, ThreadableJob*> (newValue=0x7fffe7ff8f48, _q_value=@0xffffffff40998c60: <error reading variable>)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qgenericatomic.h:111
No locals.
#1  QBasicAtomicPointer<ThreadableJob>::storeRelease (this=0xffffffff40998c60, newValue=0x7fffe7ff8f48) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qbasicatomic.h:259
No locals.
#2  0x0000000000578f14 in QAtomicPointer<ThreadableJob>::operator= (this=0xffffffff40998c60, other=...) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qatomic.h:178
No locals.
#3  0x0000000000578968 in MixerWorkerThread::JobQueue::addJob (this=0x9d1a60 <MixerWorkerThread::globalJobQueue>, _job=0x7fffe7ff8f48) at /home/umcaruje/github/lmmstest/src/core/MixerWorkerThread.cpp:57
No locals.
#4  0x0000000000555538 in MixerWorkerThread::addJob (_job=0x7fffe7ff8f48) at /home/umcaruje/github/lmmstest/include/MixerWorkerThread.h:87
No locals.
#5  0x00000000005753fa in MixerWorkerThread::fillJobQueue<QList<PlayHandle*> > (_vec=..., _opMode=MixerWorkerThread::JobQueue::Static) at /home/umcaruje/github/lmmstest/include/MixerWorkerThread.h:99
        it = {i = 0x7fff7806cf78}
#6  0x0000000000571fe5 in Mixer::renderNextBuffer (this=0xba56c0) at /home/umcaruje/github/lmmstest/src/core/Mixer.cpp:438
        last_metro_pos = {<MidiTime> = {m_ticks = -1, static s_ticksPerTact = 192}, m_timeLine = 0x0, m_currentFrame = 0}
        song = 0xe2d990
        currentPlayMode = Song::Mode_PlaySong
        p = {<MidiTime> = {m_ticks = 8212, static s_ticksPerTact = 192}, m_timeLine = 0x2a9df60, m_currentFrame = 312.87793}
        playModeSupportsMetronome = true
        it_rem = {i = 0x7fff780013b0}
        fxMixer = 0xea2800
#7  0x0000000000573c3f in Mixer::fifoWriter::run (this=0xb1f3e0) at /home/umcaruje/github/lmmstest/src/core/Mixer.cpp:1102
        buffer = 0x7fff780257e0
        b = 0xe0ce10
        frames = 256
#8  0x00007ffff4b0c7be in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
No symbol table info available.
#9  0x00007ffff7bc16ba in start_thread (arg=0x7fff859a0700) at pthread_create.c:333
        __res = <optimized out>
        pd = 0x7fff859a0700
        now = <optimized out>
        unwind_buf = {cancel_jmp_buf = {{jmp_buf = {140735434852096, 8462573384777015085, 1, 140737488344511, 140735434852800, 11662208, -8462806189127819475, -8462555276406087891}, mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 
              0x0}, data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
        not_first_call = <optimized out>
        pagesize_m1 = <optimized out>
        sp = <optimized out>
        freesize = <optimized out>
        __PRETTY_FUNCTION__ = "start_thread"
#10 0x00007ffff44223dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
No locals.
@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Sep 17, 2017

@Umcaruje That's an unrelated crash (job queue is full), see #777 (comment).

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Sep 17, 2017

Ah, ok, good, this fixes the out of buffers issue and I can't seem to pick up any issues with it. This is ready to merge.

#include "BufferManager.h"
#include "Engine.h"
#include "Mixer.h"
#include "PlayHandle.h"

This comment has been minimized.

@PhysSong

PhysSong Sep 19, 2017
Member

Why this line have been moved from above? It should stay above #include "BufferManager.h" according to our convention, and that way seems more consistent for me.

This comment has been minimized.

@lukas-w

lukas-w Sep 19, 2017
Author Member

No reason, probably an unintentional change.


typedef std::vector< T, MmAllocator<T> > vector;
};

This comment has been minimized.

@PhysSong

PhysSong Sep 19, 2017
Member

These parts aren't used. Are there some reasons you leave them in our codebase?
Some more inconsistencies: you mixed < foo > and <foo>, it makes the code look inconsistent.
One more thing(minor): if you are going to use this later, I think vector is not very good name. If you want to keep the name, of course you may do.

This comment has been minimized.

@lukas-w

lukas-w Sep 19, 2017
Author Member

Are there some reasons you leave them in our codebase?

I plan on using them at a later point in time. Using an Allocator class is the standard C++ way of having custom allocators, and should be preferred over using the macros MM_ALLOC and MM_FREE.

you mixed < foo > and , it makes the code look inconsistent.

This has already been discussed by @Umcaruje and me in the comments here. C++03 doesn't allow >> for nested template arguments (it always interprets it as the stream right shift operator >>). We can't use >> here, because the headers is included by plugins that are compiled without C++11 support. See d1aa39b.

This comment has been minimized.

@lukas-w

lukas-w Sep 19, 2017
Author Member

if you are going to use this later, I think vector is not very good name

Well it's a vector, so why not call it vector? What would you call it?

This comment has been minimized.

@PhysSong

PhysSong Sep 19, 2017
Member

You're right, but you used both template<typename T> and template< class U > above. Is that intentional?

This comment has been minimized.

@lukas-w

lukas-w Sep 19, 2017
Author Member

Ah, no that's not intentional.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 19, 2017

It seems to fix the issue clearly, anyway.

test.mmp.zip

This project file shows the bug clearly and it seems to be fixed by this PR.

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Sep 22, 2017

@lukas-w I think it is ready to merge. However, I suggest
fixing inconsistent and unintentional things before you merge. Would this pach be fine?

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Sep 22, 2017

Would this pach be fine?

@PhysSong Sure. Feel free to apply it if you find the time before I do.

@lukas-w
Copy link
Member Author

@lukas-w lukas-w commented Sep 26, 2017

Thanks @PhysSong. Merging.

@lukas-w lukas-w merged commit f23cf4e into stable-1.2 Sep 26, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@lukas-w lukas-w deleted the fix/buffer-manager branch Sep 26, 2017
@lukas-w

This comment has been minimized.

Copy link
Member Author

@lukas-w lukas-w commented on b6421d4 Sep 29, 2017

Commit message says "fixes #3670", but should say "#3223" instead.

@@ -60,6 +65,10 @@ void PlayHandle::doProcessing()

void PlayHandle::releaseBuffer()
{
BufferManager::release( m_playHandleBuffer );

This comment has been minimized.

@PhysSong

PhysSong Oct 15, 2017
Member

@lukas-w Why did you move this to destructor?

This comment has been minimized.

@lukas-w

lukas-w Oct 15, 2017
Author Member

@PhysSong Because I moved buffer allocation to the constructor. The buffer's lifetime matches the PlayHandle's lifetime now, instead of being allocated everytime doProcessing is called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.