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

Rewrite TripleBuffer as a true lockless, flipping, triple buffer #6319

Merged
merged 12 commits into from
Jun 28, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jun 28, 2024

What made me do this is over the last couple of days I encountered an assert in song select using a debug o!f:

Unhandled exception. NUnit.Framework.AssertionException: : 
   at osu.Framework.Logging.ThrowingTraceListener.Fail(String message1, String message2) in /Users/smgi/Repos/osu-framework/osu.Framework/Logging/ThrowingTraceListener.cs:line 27
   at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
   at System.Diagnostics.Debug.Fail(String message, String detailMessage)
   at osu.Framework.Allocation.TripleBuffer`1.getPendingReadBuffer() in /Users/smgi/Repos/osu-framework/osu.Framework/Allocation/TripleBuffer.cs:line 93
   at osu.Framework.Allocation.TripleBuffer`1.GetForRead() in /Users/smgi/Repos/osu-framework/osu.Framework/Allocation/TripleBuffer.cs:line 65
   at osu.Framework.Allocation.TripleBuffer`1.GetForRead() in /Users/smgi/Repos/osu-framework/osu.Framework/Allocation/TripleBuffer.cs:line 75
   at osu.Framework.Platform.GameHost.DrawFrame() in /Users/smgi/Repos/osu-framework/osu.Framework/Platform/GameHost.cs:line 514
   at osu.Framework.Threading.GameThread.processFrame() in /Users/smgi/Repos/osu-framework/osu.Framework/Threading/GameThread.cs:line 451

It can be repro'ed via:

[Test]
public void StressTest()
{
    TripleBuffer<object> tripleBuffer = new TripleBuffer<object>();

    new Thread(() =>
    {
        while (true)
        {
            using (tripleBuffer.GetForWrite())
            {
            }
        }
    }).Start();

    new Thread(() =>
    {
        while (true)
        {
            using (tripleBuffer.GetForRead())
            {
            }
        }
    }).Start();

    Thread.Sleep(100000);
}

Which is pretty scary. Either one of the two asserts in getPendingReadBuffer() can trigger. We've also been seeing some intermittent CI failures though I can't point to one immediately (happens once in a blue moon), likely related to this.

Separately from the above, I became unconvinced of the existing implementation while reading through the code. As for the immediate issue above, I can't figure out why it's happening, but even more - I've been saying for a long time now that I want TripleBuffer to evenly distribute occupancy across all buffers. I wrote a test (TestSceneTripleBufferOccupancy) to test this and found that the existing implementation breaks down almost as soon as both producers/consumers aren't running at absolute unlimited rate:

Current New
Screenshot 2024-06-28 at 09 03 18 Screenshot 2024-06-28 at 09 03 47

So instead of fumbling around with the existing implementation, I chose to reimplement it hopefully following the existing principles of triple buffering in graphics: a front/back buffer with an intermediate flip buffer.
The write buffer is flipped onto the flip buffer after the write completes, and the flip buffer is flipped onto the read buffer when a read is requested. This is also completely lockless now, compared to the current implementation which locks between the two threads.

Note that this is a custom implementation so I'm not sure if this is exactly how this is supposed to be implemented, but it makes sense in my head and seems to work in practice, so... /shrug?

@peppy peppy self-requested a review June 28, 2024 02:18
The new implementation has less safety. If someone wants to use a triple
buffer for their own purposes it's best they implement one locally so we
can keep this implementation as lean and raw as possible.
@peppy peppy merged commit b9cbabc into ppy:master Jun 28, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants