GNOME Bugzilla – Bug 772747
atomicqueue: Improve performance on push operations
Last modified: 2018-11-03 12:36:47 UTC
Created attachment 337414 [details] [review] Patch We have detected that on big pipelines involving a great number of elements and processes competing to post messages on the bus, the amount of time spent on single push operations rapidly increase, we have detected that it can even reach a magnitude of seconds. This issue is caused because the current algorithm forces processes to wait until previous writers finalize sequentially. This patch allow faster writers to finalize the push operation without waiting for slower writers. Those slow processes are responsible of updating the read index so that processes can pop data at the right position, never beyond the index imposed by the slowest writer. This issue might be related to the bug reported by Miguel París Díaz (https://bugzilla.gnome.org/show_bug.cgi?id=768668opened) who detected a long busy-waiting with gdb.
Created attachment 337415 [details] Profiling about the improvement gained with the patch Attached I've add some profiling I've done with and without the patch. When the patch is applied we can scale up to the double of proccesses. The picture show the amount of processes involved on the X axis, and depicted in the y axis it is the time consumed by push operations (measured in nanoseconds).
*** Bug 768668 has been marked as a duplicate of this bug. ***
Created attachment 337575 [details] [review] patch
What's this LOW_MEM #ifdef? This definitely needs a very thorough review to not introduce any new race conditions... ideally also more/new tests.
LOW_MEM macro seems to be used to keep the memory as low as possible. When it is defined, array of memory that is not going to be used anymore is freed (taking into account that no readers are using it), without it, tose arrays only will be released when the queue is destroyed. Even it seems to be disabled with the undef statement in the line 52 I tryed to keep the code compatible with this mode of operation. I completely agree with you that we need more eyes looking at this patch because code is complex and critical. We've been using it at Kurento at it seems to do what is expected to do, anycase, a good test bed will give us enough confidence to grant that no race condition are introduced with it.
Created attachment 338191 [details] [review] Test for atomicqueue
Hi, I added some cobeture tests for the atomicqueue. I hope this helps to speed up the inclusion of this enhancement. Regards.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/192.