Some low hanging fruits optimizaitons

[gfoidl]

From the project description: "with a focus on performance", so let's squeeze out a bit more...

---

Throw-helper pattern

This pattern uses local functions and goes like this:

if (condition)
{
    Throw();
    static void Throw() => throw new Exception(...);
}

The JIT won't inline methods that will never return (so methods that throw), and with that pattern the quite costly setup of the exception-object is moved to a differnt method.

The effect can be best seen at looking to the machine code. normal exception pattern or another example with throw-helper or another example

So without that pattern, these are huge methods that won't be inlined. By applying the throw-helper pattern, the methods become small so that inlining makes sense. Due the ifs I'm not entirely sure if the JIT will actually inline these methods, so I gave the JIT a hint by appyling MethodImpl-options.

BinaryPrimitives

Just look at the machine code to see the difference.

The JIT is able to emit special instructions (in the example for long a bswap-x86 instruction) which results in way less code that needs to be executed.

typeof(TValue)

The JIT is able to dead code eliminate branches, but only when typeof(TValue) is used directly. See this example where the whole code is eliminated and the (expected) constant value is used directly.

---

That and https://github.com/mycroes/Sally7/pull/20 was the eye-balling part of optimizations...further ones are harder to play (and the code-base looks quite good 👍🏻).

[mycroes]

Really like these changes and the samples, will do another review round on latest update ASAP.

[scamille]

Fascinating how much .NET code can be played around to get more performance out of it. Coming from a C++ environment, I do love doing that and understanding all the effects of the code you write, but I was also quite glad to leave a lot of that behind in .NET and focusing on the business logic :)

[mycroes]

From the project description: "with a focus on performance", so let's squeeze out a bit more...

Throw-helper pattern

This pattern uses local functions and goes like this:

if (condition)
{
    Throw();
    static void Throw() => throw new Exception(...);
}

The JIT won't inline methods that will never return (so methods that throw), and with that pattern the quite costly setup of the exception-object is moved to a differnt method.

The effect can be best seen at looking to the machine code. normal exception pattern or another example with throw-helper or another example

So without that pattern, these are huge methods that won't be inlined. By applying the throw-helper pattern, the methods become small so that inlining makes sense. Due the ifs I'm not entirely sure if the JIT will actually inline these methods, so I gave the JIT a hint by appyling MethodImpl-options.

One thing I was also wondering about (based on these changes and recent experience) is moving the throws to a separate class entirely. One of the main benefits is that all error messages end up in a single class, so it's easier to maintain standards across messages. It also has the added advantage of having a description in the form of the method name. I'd assume this has the same effects as the local static methods. I remember having seen this before in other projects (I believe EasyNetQ has a ThrowHelpers class as well), a quick google lead to this stackoverflow question with some nice responses about such an approach. I also really like the accepted answer from Mark Seemann (@ploeh), but that doesn't yield the same optimizations as having the throw externally. Given the performance focus I'm totally fine with this tradeoff.

Also, when actually naming the methods consistently we don't have to worry about i.e. ThrowInvalidTpktLengthReceived showing up at the top of the stack trace, it'll actually be really informative.

I don't see any reason to include above into this commit, but I would appreciate if you guys can share your thoughts on this subject.

[mycroes]

From the project description: "with a focus on performance", so let's squeeze out a bit more...

Also love it that you call this low hanging fruits, I'm happy you consider this low-hanging fruit but while I actually spend attention to implementation detail I never really looked as far as this. I take pleasure from reading the performance improvements created in every recent new .NET version, but they didn't help me in spotting these improvements.

BinaryPrimitives

Just look at the machine code to see the difference.

The JIT is able to emit special instructions (in the example for long a bswap-x86 instruction) which results in way less code that needs to be executed.

I'd like to think these weren't available at the time of writing, but my guess is they were 😄. I mostly compared against BitConverter, but I never knew BinaryPrimitives could bring this improvement.

typeof(TValue)

The JIT is able to dead code eliminate branches, but only when typeof(TValue) is used directly. See this example where the whole code is eliminated and the (expected) constant value is used directly.

Where did you learn about all this stuff? I guess it makes sense JIT can optimize the constant expression and can't optimize from variable usage, but from a coding perspective it seems to be the logical choice to store typeof(...) in a variable even if just to use less code in all the branches.

That and #20 was the eye-balling part of optimizations...further ones are harder to play (and the code-base looks quite good 👍🏻).

Thanks for the code-base compliment. There's decisions I doubt, but the benefits of using Sally7 over Siemens Sapi S7 have been huge for us, so I don't need to regret any choice from that perspective. I'm really happy with all your findings. I intend to write more PLC communication libraries, so I can put all this knowledge to great use there.

[gfoidl]

moving the throws to a separate class entirely

In short: I'm fine with that suggestion, and will push a commit for this soon.

I'd assume this has the same effects as the local static methods.

Yeah, has the same effect.

---

In my project I follow this rough guideline for the throw-helpers (on a per "exception-type" basis):

• if it's used only once in a method -> local function
• if it's used several times in a class -> private static method
• if it's used in several classes -> separate ThrowHelper-class

is that all error messages end up in a single class, so it's easier to maintain standards across messages

This is actually a good point. Also in regards of potential localization / globalization tasks. A counter point is that the code doesn't have great locality anymore. Take e.g. https://github.com/mycroes/Sally7/blob/263de5c70ce8ddee1f21a68ea576a9f7ea931533/Sally7/Protocol/IsoOverTcp/Tpkt.cs#L19-L35 where all code is within the same method and file. With a separate class one has to navigate to two files.

But (now) I think the benefits of having a separate class is preferable (here).

[gfoidl]

Where did you learn about all this stuff?

The typeof(T) is from following PRs in the dotnet/runtime-repo and trying the code with https://sharplab.io/ and or looking at generated machine code from BenchmarkDotNet output.

I guessed that storing the type in a variable would trigger that optimization too, but it proved wrong. So the JIT will actually only CSE and dead-code eliminate when typeof(T) is used explictely. There's room for improvement in the JIT.

I intend to write more PLC communication libraries, so I can put all this knowledge to great use there.

Looking forward to read about these, and feel free to ping me on any questions / concerns you have.

[gfoidl]

but I think I'll be able to merge and release on Wednesday at the latest.

I'm out of office until Wednesday, soll will address your points then. This means also you have enough time with the release 😃 (didn't read other points now, will do on Wed)

[gfoidl]

I prefer to have internal classes with public methods

So do I (for the same reasons). Do you refer to the internal methods from the exceptions? The exception types are public, because user-code should be able to catch them. The static factory methods are an implementation detail, so they are internal.

unsure about creating several exception types.

This and naming is among the hardest parts of programming (for me). I don't know what's the right balance here. Maybe just a Sally7Exception is enough. My motivation for the several exception types is that they can be logically grouped more easily, and with logging-system the exception type is reported so it allows for easier distinction where the error comes from. As said it's hard to meet the right balance between to fine grained and being too coarse.

If it doesn't matter if a user catches Sally7Exception or any derived exception type, so we could keep the grouping and make the derived types internal. This adds another possibility that we have, but doesn't make the problem easier...

I hope you can make a decision -- or for the moment we keep it as is, and revisit that exception-part later again. As long (according to SemVer) 1.0.0 isn't reached we should be free to change some things again.

All in all, still very pleased with this PR, also with the latest commits.

Thanks 😃

... and release

Please wait a bit with the release, as I have another PR that mainly reduces allocations (based on top of this PR, and I don't want to push these changes into this branch, as it will get quite messy then).

[mycroes]

I prefer to have internal classes with public methods

So do I (for the same reasons). Do you refer to the internal methods from the exceptions? The exception types are public, because user-code should be able to catch them. The static factory methods are an implementation detail, so they are internal.

Yes it was about the internal methods. Even if I would take that as a rule, there's always exceptions to the rule. Again this isn't something that worries me, but I'm definitely interested in opinions on this subject.

unsure about creating several exception types.

This and naming is among the hardest parts of programming (for me). I don't know what's the right balance here. Maybe just a Sally7Exception is enough. My motivation for the several exception types is that they can be logically grouped more easily, and with logging-system the exception type is reported so it allows for easier distinction where the error comes from. As said it's hard to meet the right balance between to fine grained and being too coarse.

If it doesn't matter if a user catches Sally7Exception or any derived exception type, so we could keep the grouping and make the derived types internal. This adds another possibility that we have, but doesn't make the problem easier...

The question is how we're helping consumers of the library. Is Sally7Exception useful? Is Sally7CommunicationSetupException useful? I'd say yes, there's definitely something to be gained by distinguishing some exceptions. For one, it would probably actually be possible to detect if get/put permissions are missing (which I think is error code 0x81 0x04), and that's one that has come up a couple of times for S7NetPlus for instance. I don't have vision on how we should do the exceptions, but I think that we've (or you, effectively) definitely improved the codebase by extracting them.

I hope you can make a decision -- or for the moment we keep it as is, and revisit that exception-part later again. As long (according to SemVer) 1.0.0 isn't reached we should be free to change some things again.

For now we keep it as is. As for SemVer, I intentionally haven't made the 1.0.0 jump, but I'm not afraid about making major version jumps because of improvements.

... and release

Please wait a bit with the release, as I have another PR that mainly reduces allocations (based on top of this PR, and I don't want to push these changes into this branch, as it will get quite messy then).

No worries, I'll wait for a bit. Releasing is a matter of minutes though, I tag manually and manually push to NuGet from AppVeyor, but other than that it's automated.

[gfoidl]

Thanks. Very valuable conversation -- in essence: I like the collobaration here ❤️

[mycroes]

Thank you, really appreciate your contributions and your mindset!