Add double-buffering for concurrent flushing.

[dbuse]

This PR adds a second event buffer of the same size for concurrent flushing. On flush, the buffers are swapped and the global counter is reset (both protected by the mutex.) Flushing than exclusively works on the swapped buffer, so that tracing can continue concurrently. The call to flush could then (optionally) happen in a separate thread. This effectively doubles the amount of memory needed for buffering.

I built this for use in my own work, but maybe someone else could use it, too. So I'm putting this PR up as a draft to allow discussion. We could also keep both options (with/without double buffering), separated by #ifdef-directives.

[hrydgard]

Yeah, this seems like the way to go, pretty much. I'm guessing you write a lot of events :)

Added a few minor comments.

[dbuse]

Thanks for the quick response.

I'm guessing you write a lot of events :) Yeah, north of a GB of JSON per minute...

My original solution actually contained a race: As both tracing and flushing only locked the mutex in the beginning, the two could overlap if, e.g., :

1. tracing started first, grabbed a pointer to a record in the buffer and released the mutex
2. then flushing took over, swapped the buffers (even with the mutex)
3. tracing continued to(partially) write in the record, which now points into the flushing buffer

Solving this with more locking did not seem to improve anything to me:

• either the whole flushing would have to lock, which made all flushing and tracing sequential again
• or all tracing would have to lock, which also would make all flushing and tracing sequential again

So only copying could help us:

• either construct all trace records on the stack and then copy them over (many many small copies in tracing code)
• or flushing would have to copy all (count) records from the main buffer to the tracing buffer (few big copies in flushing code)

I decided to go with the second option, as this avoid putting more delay on the tracing portion. I also added a second mutex to guard against two concurrent flushes.

[Liastre]

Hello @hrydgard, Is it going to be merged any soon?

[Liastre]

Hey @dbuse I've took your idea with double buffering and applied threadsafe implementation without copying (probably that's why @hrydgard was not optimistic about your changes). Flushing and tracing are sequential! I've used atomic like solution to check if flushing is in progress, and running only a single flush at any time. If you have a chance can you take a look on your project if it works fine?

Branch is: https://github.com/Liastre/minitrace/tree/feature/concurrent-flushing

[dbuse]

Hi @Liastre Are you sure that your solution avoid the race condition I described above (https://github.com/hrydgard/minitrace/pull/17#issuecomment-491807363)?

Flushing and tracing are sequential! How did you get to that conclusion? Maybe I missed something, but I would sequential flushing and tracing not destroy the need of double buffering? Or put differently: aren't we explicitly trying to make them non-sequential? Anyway, nice to see someone picks this up :)

[Liastre]

@dbuse hello! Yep you were right, I missed just the same point, in case tracing already acquired the pointer and then flush swapping buffers. But I've applied the fix. How it basically works now:

• I'm not locking the whole flush, only the part with swapping. Also using the flag to check if any flushing have started, so only a single flush process may be running at time.
• With applied fix, now I have counter of events in process, wrapped with a second mutex (in C++ I've used atomic int). So now flushing locks main mutex (no any new events may spawn) and waits while counter will become 0 (waiting all events which passed main mutex and being at process).

So currently there a very small part of code is a critical section, few lines of swapping buffers and counter changes. All events may be spawned async and flush may be called any amount of time but if there any flush in progress it will return immediately.

[Liastre]

@hrydgard this one might be closed as well, fixed by https://github.com/hrydgard/minitrace/pull/24