search

dave-hillier / disruptor-unity3d

Basic implementation of Disruptor for Unity3d
Apache License 2.0
200 stars 35 forks source link

Incorrect data dequeued in Android and iOS #5

Closed Petrakeas closed 8 years ago

Petrakeas commented 8 years ago

I added a validation check in the benchmark to make sure that the data dequeued are the ones that were enqueued. Unfortunately, when ran in Unity in Android or iOS sometimes the data dequeued are older than what they should be. (Please see my pull request: https://github.com/dave-hillier/disruptor-unity3d/pull/4 )

This is because of 2 reasons:

The Memory Barriers used in RingBuffer do not prevent reordering of read and write instructions when accessing _entries and the producer cursor. I have created a pull request that makes sure that the producer writes the new value in _entriesand then updates the producer cursor (to signal that the data are ready to be used). The consumer first reads the producer cursor to make sure that the data are ready and then reads the actual data stored in _entriesto make sure it reads the latest value.

2.

The Mono runtime used in Unity seems to not handle the Memory Barriers correctly. The original RingBuffer and the corrected one in my pull request produce wrong results. I have tested the RingBuffer with the same test via Xamarin (which uses the latest Mono runtime) in Android and iOS. Both versions work correctly in Xamarin.

I have opened a ticket in Unity regarding the Memory Barrier implementation in Unity's Mono back-end: https://issuetracker.unity3d.com/issues/android-ios-thread-dot-memorybarrier-does-not-prevent-instruction-reordering-with-mono-backend

I would refrain from using this lock-free ring buffer in Unity at the moment.

Looking forward to your thoughts on this.

dave-hillier commented 8 years ago

Does Unity use IL2CPP for iOS. Until C++11 I didnt think there was any cross-platform memory model stuff?

dave-hillier commented 8 years ago

For me, the lock free part was kind of a bonus; I was really after a structure that did not allocate. It might be worth in those use cases using a different method for concurrency control.

Petrakeas commented 8 years ago

You can select between IL2CPP and Mono 2x for iOS. I don't know how they have implemented the memory model in C++. Perhaps they are using some library instead of the new memory fence and other primitive introduced in C++11.

From my tests, IL2CPP works correctly but is quite slow. Mono 2X has a bug in Memory Barriers and the test fails. I left a ticker in Unity.

I tested your code (and my pull request) in Mono 4.5 using Xamarin. They work correctly. Unfortunately, Unity uses Mono 2X which hasn't solved that bug yet. So, the lock-free code will not work correctly in iOS and Android until they solve the bug. It works correctly on X86 machines though.

dave-hillier commented 8 years ago

I'll leave this issue open until they resolve it (even if I merge your PR)

Petrakeas commented 8 years ago

I have implemented a circular buffer like yours (that does not make allocations) but uses locks instead of memory barriers. Locks are not as fast as this lock-free implementation but unless you are transferring 1000 items/sec it won't really matter.

dave-hillier commented 8 years ago

Locks are pretty fast; the thing that isn't fast is contention. This RingBuffer is fast because readers and writers can be concurrent and dont have to wait on each other.

Petrakeas commented 8 years ago

You're right! So, locks at the moment for Unity :/

Petrakeas commented 8 years ago

The bug has been fixed in Unity 5.5, since it uses Mono 4.4: https://issuetracker.unity3d.com/issues/android-ios-thread-dot-memorybarrier-does-not-prevent-instruction-reordering-with-mono-backend

The test works correctly now.