-
Notifications
You must be signed in to change notification settings - Fork 94
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
Structs vs Classes #31
Comments
I agree there may be places where structs make more sense. The reason everything is a class at the moment is just a direct port from the Java and there are no allocation issues yet. At the moment once everything is initialized and the program is in a publish/subscribe loop, there are no allocations in the samples (except for a bug IpcThroughput which is fixed). And you're right, the intention was to reuse objects as much as possible. As we start to use the client a bit more I'll review where structs might be more useful. I've separated the call vs callvirt in into a separate issue #33 |
Hi Victor, Regarding struct vs class there are certainly places where it would make sense. I ported the disruptor to .NET a while ago and also SBE and one thing I learned during those exercise is that on the long run, the closer you keep the implementation from the original source, the easier it is to maintain. When I ported the disruptor I spend a fair amount of time replacing some data structures with structs, which sometime lead performance benefits, but also increased the effort when porting new features from the Java codebase. The approach we are taking with Aeron was first to do a very close port (that's done I think) and then we can think about optimizing where we see fit (that's what we can do now and your input is very welcome!). Regarding virtual calls, UnsafeBuffer did not have a single virtual method initially when I ported it but it became a probleme when we started porting the tests. Most (if not all?) mocking frameworks use castle dynamic proxies under the hood and methods need to be virtual for this to work. There might be other mocking frameworks out there which do not bring those constraints. As previously mentioned, we are trying to get the port correct first, and we think it's important to also be able to port all the tests. If you check the Aeron tests, you will see that there are loads of tests using mocks of UnsafeBuffer. If we can figure out a way to keep the tests without virtual method calls I would be very happy to make the change and make UnsafeBuffer sealed again. Anyway, it's great to get feedback and if you have some ideas and want to do some experiments this is very welcome. |
I will look closer to the Aeron tests. Currently Aeron.NET's UnsafeBuffer methods are already sealed and are not used in any test directly (there is a recent commit where virtual was removed). My first thought was that we just need to mock IntPtr via Marshal.Allochglobal, because the buffer is just a view into some memory region, the methods behavior should be the same in tests. |
Java's lack of of structs (Value Types) should not hold back the .NET client from having a better implementation that is more idiomatic to C#. |
Interestingly, simply making UnsafeBuffer a struct kills performance to 10 Mops. I have read somewhere that JIT does not inline struct methods as aggressively as ones from classes. Or buffers-as-structs are passed by value too often in Aeron so that a simple change from a class to a struct is not enough. Before this exercise I had misconception that structs are universally superior due to GC. |
I wonder why you do not use structs vs classes where it makes code more idiomatic to .NET and helps to avoid allocations? I see that you have classes vs structs on some hot paths. E.g.
BufferClaim
is a class.UnsafeBuffer
is also a class, however both are very close toArraySegment<byte>
by usage semantics. There is a mantra that short-lived objects are very cheap to allocate and collect in .NET, but the costs are not zero.Or maybe your intent is to reuse objects when possible and use
Wrap()
methods, as commented here? E.g. you mutateBufferClaim
that is passed to a method and then use the same instance in a loop. But what if we want to claim space for a result of some work, and then complete this work asynchronously using a task? We could pass a struct easily by value between threads in such a scenario, but cannot reuse a class and have to allocate. The same is true forUnsafeBuffers
- if they are structs, we could create as many instances as convenient for free, and still reuse a single instance if we want to do so.Additionally, there is
call
vscallvirt
and interface method call minor issue. In JVM, everything is an object and probably method calls are optimized more than in CLR. However, inside tight loops I have noticed that interface method calls are noticeably slower than direct method calls. In theDirect/UnsafeBuffer
case, all we do is writing to/reading from a pointer in unsafe code, e.g.PutLog
, etc. Usually this is a very simple method with a couple of instructions, and then method call overhead becomes visible. That could matter in IPC case for Aeron, less so for networking. In your implementation, there is only one implementation of different buffer interfaces, so, at this stage, class hierarchy adds only overheads. Also,UnsafeBuffer
currently works both withbyte[]
andIntPtr
, and a single concrete implementation as a struct will be the fastest. (I separated them in my implementation initially, but will soon merge back to avoid using interfaces altogether).I have just done a trivial test to show this. At first, I tested integer increment (i++), and struct method call was completely eliminated/inlined. In a more realistic scenario, structs do not differ much from sealed classes, but interface overheads are clearly visible.
Struct vs classes was the only change I did while porting LogBuffers. My implementation of
DirectBuffer
was originally based on aDirectBuffer
C# port in SBE by Adaptive (which, in turn, was the same port from Argona), but I then reworked it for the reasons described above.The text was updated successfully, but these errors were encountered: