Serialize Tpkt, Data and Header using Unsafe.WriteUnaligned

[mycroes]

Thanks again @gfoidl. Very nice that you jumped straight in, will keep the comments in mind for additional changes I intend to make as well.

[mycroes]

Left some comments. FYI: I'll be back on Monday -- wish you a nice weekend.

A nice weekend to you as well! No need to hurry with reviewing, I'm not in a hurry to make these changes either. I'm aiming for GitHub activity every day, just so that I'll keep working on my projects and perhaps start some new projects too. I don't necessarily expect you to join my effort 😉

[scamille]

Interesting PR. While I am usually quite fond of doing optimizations, I am wondering if this really make a difference for such heavy I/O bound applications like you have with Sally7, even if you have tons of concurrent connections.

It also is sad that you have to go through these extra steps and call Unsafe operations to squeeze out that extra performance :-) In particular since it seems to sacrifice some of the nice Span interface in favor of decoupled pointer/length arguments.

[mycroes]

Interesting PR. While I am usually quite fond of doing optimizations, I am wondering if this really make a difference for such heavy I/O bound applications like you have with Sally7, even if you have tons of concurrent connections.

That's a fair question. Performance wise there's quite some gain in the processing time, that doesn't change the I/O bounds. Still it could result in overall lower response times. Let's say it's better for the environment because we waste less CPU cycles. 💚

It also is sad that you have to go through these extra steps and call Unsafe operations to squeeze out that extra performance :-) In particular since it seems to sacrifice some of the nice Span interface in favor of decoupled pointer/length arguments.

I'm not sure if Unsafe operations is the right definition. It's actually different from using the unsafe keyword, though I guess the effects are roughly the same in the sense that you could have buffer overflows / underflows.

And yes, one of the features of the library was the fact that everything was mapped to structs and enums. I actually still have the following sentence in the README:

All protocols are mapped to structs and enums with the intent to create a project that is easy to comprehend and extend.

I'm going to divert from that approach. Basically because I'm dictating the direction of Sally7 and it's up to me to chose, but also because I value the performance gains. For me personally it's also unsellable that I'm writing more performant code in the next library (AdsClient) while performance is the main feature of Sally7. However (and that's a BIG however), I don't want to turn this into a project that is unreadable, unmaintainable and in the end unusable. This should be clean code and still have a relatively low barrier of entrance, it just won't be using structs for that in the long term.

I also invested a bit of time in unit tests, I want to expand on that. I'd like to add server side support as well at some point, not sure yet how I can do that while sharing as much of the code as possible. Maybe I will end up using structs and code generation to get the best of both worlds. 😆

Either way, thanks for your comments, again dearly appreciated! ❤️

[scamille]

That's a fair question. Performance wise there's quite some gain in the processing time, that doesn't change the I/O bounds. Still it could result in overall lower response times. Let's say it's better for the environment because we waste less CPU cycles. 💚

Processing time improvement can definitely be worthwhile - it just never was that big of a deal for the projects I worked on back in the days, since one side talked to a database and the other to a S7 PLC :-)

And yeah it is both interesting in how you can squeeze more performance out of .NET, and sad that you have to know these special tricks at the same time.

The AdsClient looks quite interesting. I worked on a small project using TwinCat at some point, and the library from Beckhoff that was already in place seemed to be okay. But that was with local communication only anyway. Would you mind sharing (and maybe adding to the Readme) what the motivation was for creating your own library?

[mycroes]

That's a fair question. Performance wise there's quite some gain in the processing time, that doesn't change the I/O bounds. Still it could result in overall lower response times. Let's say it's better for the environment because we waste less CPU cycles. 💚

Processing time improvement can definitely be worthwhile - it just never was that big of a deal for the projects I worked on back in the days, since one side talked to a database and the other to a S7 PLC :-)

Yeah I guess Viscon is by far the biggest user of Sally7 if you factor in the number of requests. Then again our abstraction layer still needs some serious work as well, that's not allocation free at all...

And yeah it is both interesting in how you can squeeze more performance out of .NET, and sad that you have to know these special tricks at the same time.

I especially love all the details Günther is providing. I'm just using API's that are there, Günther is actually giving advice that squeezes every last bit of performance out of the CLR.

The AdsClient looks quite interesting. I worked on a small project using TwinCat at some point, and the library from Beckhoff that was already in place seemed to be okay. But that was with local communication only anyway. Would you mind sharing (and maybe adding to the Readme) what the motivation was for creating your own library?

Let's call it ignorance 😄 I'm not familiar with Beckhoff PLC's, but we had another supplier at a customer that uses Beckhoff PLC's and we needed to do some communication. We asked their preferred form of communication and it was ADS. Searching for solutions I found the AdsClient library (I didn't write this from scratch). I think I did see NuGet packages for the Beckhoff library, but because I couldn't find any more information (should've just clicked the link on nuget.org though) I was a bit uncertain about using it.

So then I started by contacting the maintainer of the AdsClient library and got the GitHub repository transferred to me. The NuGet package was still linked to the original author though. He no longer had control due to account changes on nuget.org, but with a lot of communicating back and forth with NuGet admins I got that transferred too (not actually using it now though).

With all that out of the way I started doing some initial communication tests. That didn't go as planned at first, but soon enough we figured it was a similar issue to Siemens where both the IP and identifiers have to match. I could borrow a PLC though, so that allowed me to test locally and refactor the library into what it is now. There's still a lot of room for improvement there as well and just like Sally7 it's a bit short on unit tests, but it was fun to do and it works pretty well. Of course I still need to compare it against Beckhoff's library to see which one performs better and if it's the Beckhoff library I've got some work to do. Final thing I need to handle is multiple connections, which apparently is constrained to a single connection per IP, so I will be adding a proxy that solves that problem and is actually different from the router as provided by Beckhoff as well.

Long story short, I'm not sure I needed to write the library. But I did, and while doing that I even fixed a bug in the NodeJS ads-client package as well (which I also used for reference). And I got to know about Unsafe.WriteUnaligned, Marshal.GetReference and sneaky Unsafe.As usage. So a big win for me and a good reason to start some new work on Sally7!

[mycroes]

@gfoidl care to give this your final verdict? I preferably want to merge this PR before I start refactoring other parts of the code.