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 #20 was the eye-balling part of optimizations...further ones are harder to play (and the code-base looks quite good 👍🏻).

[gfoidl]

This will actually rent an array of size rounded up to the next power of two, so the length will be 128.
This shouldn't be problem, as the implementation in the S7Connection is defensive for that.

[gfoidl]

This is just a mini-optimization. By flipping the order (the largest index first), the JIT only needs to emit one bound check instead of two.

[mycroes]

I love these insights!

[gfoidl]

We can avoid the allocation of the temporary array on NS 2.1 or newer (there won't be any newer anyway for .NET Standard as .NET 5 onwards is the future).

[mycroes]

Excellent, for now I guess it's still a good idea to keep compatibility with older targets.

[gfoidl]

I'm not sure if this is overkill here...
Is string-handling in PLC that common? I don't know.

Anyway...if the string is short enough we stack-allocate, otherwise just do a regular allocation. One could rent a buffer from the array-pool, but this makes the code a bit more unreadable though.

[mycroes]

Not sure what you mean by string-handling in PLC being common. I'm actually not sure what the encoding is, because it might just as well be UTF-8 for normal strings, but I'm quite certain there's also a UTF-16 string type in S7-1200 and 1500 (from documentation, I don't program PLC's). Let's say there's room for improvement in this area (in the original code).

[scamille]

Normal strings depend on the language set in the PLC project as far as I know. Latin1 or even something more specific is definitely used for german plc projects, but unfortunately there isn't any good documentation on all the encodings used, at least not online or anywhere i tried to look. (S7 documentation in general is so awful!)

So I would definitely recommend sticking to ASCII here. WString uses utf-16, but I haven't seen that used in the wild ever.

[gfoidl]

what you mean by string-handling in PLC being common

Pure speculation by me, that PLCs are used for short, float, etc. and not strings. But I'm not a PLC programmer. As @scamille writes, strings are used, so my speculation seems to be wrong.

What if there's an "option" that allows to specify the encoding used? With a default value if not specified of ASCII. So any user could define "use Latin1" or "use UTF-8" as encoding.

We can't plumb that into the conversion directly, but as possibilities:

• static property on a config-class -- the user can set that value before the connection is constructed
• environment variable -- similar to some knobs used in .NET itself
• AppContext -- don't know if this works for .NET 4.x too

I'd prefer one of the two former bullets.

[mycroes]

It would even be possible to permit encoding on a per DataItem basis, although I guess I should make some changes here and there to make that comfortable for users. I'm not a big fan of static properties on a config class, because that doesn't permit fine-grained control. All in all though, mixing the two might work out quite well. Another option would be to have a factory for DataItems that is tied to the PLC or a configuration object.

With regards to strings in PLC, at Viscon they're used for barcodes and sometimes for job details (either PLC having job configurations with names that are also visible in HMI, or the software sending job information for the active job to display on the HMI). We're currently getting away with ASCII only, but I know it's not flexible.

[gfoidl]

Ah, thanks for the use-case of strings -- that makes sense.

Static config isn't my absolute favorite too, but as current code looks like

Sally7/Sally7/ValueConversion/ToS7Conversions.cs

Line 57 in d7e5b45

if (type == typeof(string)) return new ConvertToS7<string>(ConvertFromString);

I feel it's not that easy to plumb the encoding into it.

TBH: I would leave the code as is -- i.e. stick to ASCII -- until someone requests to specifiy a different encoding in the sense of YAGNI. Or at least we should / could discuss this in a separate issue.

[mycroes]

I agree. I don't mind that the discussion came up here, but I don't see a need to change it at this point.

[mycroes]

I like this one. While there's actually some shift-right or shift-left of 3 bits in the address calculations I never figured this could've done the same.

[mycroes]

I love these insights!

[mycroes]

I'm quite sure I actually used this, not sure if it was in Sally7 😆

[mycroes]

Missed / 8 to >> 3 optimization

[gfoidl]

😉 not really missed -- in the presence of the allocation and of Linq this micro-optimization won't have any effect.

I'm sure this whole method can be optimized, just need to figure out (and TBH need to understand how the BitArray works, didn't use that type before).

[gfoidl]

I'm working on this now. Seems there's an quite easy way 😉

[gfoidl]

Numbers look great:

|    Method |      Categories |      Mean |    Error |    StdDev | Ratio | RatioSD |  Gen 0 | Allocated |
|---------- |---------------- |----------:|---------:|----------:|------:|--------:|-------:|----------:|
|   Default | ConvertFromBool |  55.88 ns | 1.098 ns |  2.292 ns |  1.00 |    0.00 | 0.0305 |      96 B |
| Optimized | ConvertFromBool |  20.46 ns | 0.448 ns |  1.065 ns |  0.37 |    0.02 |      - |         - |
|           |                 |           |          |           |       |         |        |           |
|   Default |   ConvertToBool | 425.41 ns | 9.007 ns | 26.557 ns |  1.00 |    0.00 | 0.1912 |     600 B |
| Optimized |   ConvertToBool |  14.73 ns | 0.350 ns |  1.032 ns |  0.03 |    0.00 |      - |         - |

Benchmark code

using System;
using System.Collections;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Configs;
using BenchmarkDotNet.Running;

Bench bench = new();
{
    bench.GlobalSetup();
    Console.WriteLine("ConvertFromBool:");
    Console.WriteLine("================");
    bench.ResetBytes();
    bench.ConvertFromBool_Default();
    bench.Print();
    Console.WriteLine();
    bench.ResetBytes();
    bench.ConvertFromBool_Optimized();
    bench.Print();
}
Console.WriteLine();
{
    bench.GlobalSetup();
    Console.WriteLine("ConvertToBool:");
    Console.WriteLine("==============");
    bench.ResetBools();
    bench.ConvertToBoolArray_Default();
    bench.Print();
    Console.WriteLine();
    bench.ResetBools();
    bench.ConvertToBoolArray_Optimized();
    bench.Print();
}

#if !DEBUG
BenchmarkRunner.Run<Bench>();
#endif

[MemoryDiagnoser]
[CategoriesColumn]
[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)]
public class Bench
{
    private bool[]? _bools;
    private byte[]? _bytes;

    [GlobalSetup]
    public void GlobalSetup()
    {
        _bools = new bool[] { true, false, false, true, true, false, true, true, true };
        _bytes = new byte[] { 137, 1 };
    }

    [Conditional("DEBUG")]
    public void Print()
    {
        foreach (bool b in _bools!)
        {
            Console.Write($"{(b ? "T" : "F")} ");
        }
        Console.WriteLine();

        BitArray bitArray = new(_bytes!);
        foreach (bool b in bitArray.Cast<bool>().Take(_bools.Length))
        {
            Console.Write($"{(b ? "T" : "F")} ");
        }
        Console.WriteLine();
    }

    [Conditional("DEBUG")]
    public void ResetBools() => _bools.AsSpan().Fill(true);

    [Conditional("DEBUG")]
    public void ResetBytes() => _bytes.AsSpan().Fill(0x55);

    [Benchmark(Baseline = true, Description = "Default")]
    [BenchmarkCategory("ConvertFromBool")]
    public int ConvertFromBool_Default() => ConvertFromBool_Default(_bools, _bools!.Length, _bytes);

    [Benchmark(Description = "Optimized")]
    [BenchmarkCategory("ConvertFromBool")]
    public int ConvertFromBool_Optimized() => ConvertFromBool_Optimized(_bools, _bools!.Length, _bytes);

    [Benchmark(Baseline = true, Description = "Default")]
    [BenchmarkCategory("ConvertToBool")]
    public void ConvertToBoolArray_Default() => ConvertToBoolArray_Default(ref _bools, _bytes, _bools!.Length);

    [Benchmark(Description = "Optimized")]
    [BenchmarkCategory("ConvertToBool")]
    public void ConvertToBoolArray_Optimized() => ConvertToBoolArray_Optimized(ref _bools, _bytes, _bools!.Length);

    private static int ConvertFromBool_Default(bool[]? value, int length, Span<byte> output)
    {
        if (value == null) throw new ArgumentNullException(nameof(value), "Value can't be null.");

        var bitArray = new BitArray(value);
        var byteArray = new byte[(length + 7) / 8];
        bitArray.CopyTo(byteArray, 0);
        byteArray.CopyTo(output);

        return byteArray.Length;
    }

    private static int ConvertFromBool_Optimized(bool[]? value, int length, Span<byte> output)
    {
        if (value is null)
        {
            Throw();
            [DoesNotReturn]
            static void Throw() => throw new ArgumentNullException(nameof(value), "Value can't be null");
        }

        length = (length + 7) >> 3;     // (length + 7) / 8

        int outputIdx = 0;
        int bitIdx = 0;

        foreach (bool b in value)
        {
            if (b)
            {
                output[outputIdx] |= (byte)(1 << bitIdx);
            }
            else
            {
                output[outputIdx] &= (byte)~(1 << bitIdx);
            }

            bitIdx++;

            if ((bitIdx & 7) == 0)
            {
                outputIdx++;
                bitIdx = 0;
            }
        }

        return length;
    }

    private static void ConvertToBoolArray_Default(ref bool[]? value, ReadOnlySpan<byte> input, int length)
    {
        value = new BitArray(input.Slice(0, (length + 7) / 8).ToArray())
            .Cast<bool>()
            .Take(length)
            .ToArray();
    }

    private static void ConvertToBoolArray_Optimized(ref bool[]? value, ReadOnlySpan<byte> input, int length)
    {
        value ??= new bool[length];
        input = input.Slice(0, (length + 7) >> 3);      // (length + 7) / 8

        int valueIdx = 0;

        foreach (byte b in input)
        {
            for (int i = 0; i < 8; ++i)
            {
                if ((uint)valueIdx >= (uint)value.Length)
                {
                    return;
                }

                value[valueIdx++] = (b & (1 << i)) != 0;
            }
        }
    }
}

I'll add some unit tests to this project for that, and hopefully there's no bug in it. If not, the commit will be pushed.

[gfoidl]

-> eb0f4f0

(next is to move the throw helper into the specific class)

[mycroes]

Excellent, for now I guess it's still a good idea to keep compatibility with older targets.

[mycroes]

I guess the maxByteCount is the actual number of bytes (value.Length) used in the PLC. Also, AFAIK all ASCII characters are 1 byte only (contrary to unicode). I believe string length is defined in a single byte, so it's always <256. I think it's even lower, possibly 254 to get it to end on an even byte.

[gfoidl]

I believe string length is defined in a single byte, so it's always <256

I think so too, but also I'd like to be defensive -- or if in the future there's a PLC that supports string lengths of WORD size -- so it's good to have a backup-plan for bigger sizes, that's why in that case there is the allocation for the buffer (on the heap).

Shall I remove the defensive part and solely use the stack-allocated buffer?

[mycroes]

There can't be a PLC that supports > 255 byte strings unless they change the storage for the header, so I'd say stackallocing 256 is perfectly fine. I was mistaken about the length though, because it's the C# string that gets measured and I'm not sure how it treats unicode chars of more than 1 byte when getting bytes using ASCII encoding, then again that doesn't keep me awake at night.

[gfoidl]

how it treats unicode chars of more than 1 byte when getting bytes using ASCII encoding

See sharplab
It get's decoded to ?s.
But for encoding the string -> bytes, it may expand to more than a 1:1 mapping.

can't be a PLC that supports > 255 byte strings unless they change the storage for the header

In the case of encoding this would throw as output (the span) is too small in ConvertFromString or data will be truncated.

that doesn't keep me awake at night

I see this similar. One should know how treat strings in regards to PLCs, so nobody can blaim this project if one shoots itself in the foot...

[mycroes]

Not sure what you mean by string-handling in PLC being common. I'm actually not sure what the encoding is, because it might just as well be UTF-8 for normal strings, but I'm quite certain there's also a UTF-16 string type in S7-1200 and 1500 (from documentation, I don't program PLC's). Let's say there's room for improvement in this area (in the original code).

[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.

Sally7/Sally7/Protocol/IsoOverTcp/Tpkt.cs

Lines 19 to 35 in 263de5c

	if (Version != IsoVersion)
	{
	Throw();
	static void Throw() => throw new Exception("Spec violation: TPKT header has incorrect version.");
	}
	if (Reserved != 0)
	{
	Throw();
	static void Throw() => throw new Exception("Spec violoation: TPKT reserved is not 0.");
	}
	if (Length.High == 0 && Length.Low < 7)
	{
	Throw();
	static void Throw() => throw new Exception("Spec violation: TPKT length is smaller than 7.");
	}

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]

Hm, somehow I like it, but somehow not really.

Maybe we should introduce specific exceptions (per "category") put the throw helpers there?
Something like

public Sally7Exception : Exception
{
    ...
}
public AssertException : Sally7Exception
{
    public static void ThrowFailLengthReceived(...);
    ...
}
public CommunicationException : Sally7Exception
{
    public static void ThrowInvalidJobID(...);
    ...
}
...

What do you think?

[mycroes]

Maybe group them per part of protocol, like TpktExceptions.ThrowOnlyClassZeroSupported, CommunicationSetupExceptions, ... I definitely prefer a bit more separation, I'm also quite baffled by how many exceptions are in Sally7. That's great though, I can usually tell what's going on just from the exception message, although I probably haven't seen 90% of these exceptions when communicating with a real PLC.

[gfoidl]

per part of protocol

Makes sense. I'm not really aware of the protocol parts, so I'll give it a try, then be happy if you can help me to arrange the things (exceptions) to the correct part if they don't fit.

[gfoidl]

My suggestion 25b0215
I'm not sure if I met correct "categories" (due to lack of knowledge of the protocol), so please feel free to edit and push to this PR as you like.

[mycroes]

There's a couple of things I would've done differently, but I'm just putting them here for discussion, not as a change request. The change request is for the typo and optional change of Sally7SetupException name.

• I prefer to have internal classes with public methods. It's just a rule of thumb that permits easy change of visibility for the entire class from internal to public. Not sure how relevant it is here, because I don't see a purpose of making these public (however, in the context of an S7 server, there could be).
• I'm unsure about creating several exception types. I could understand a single exception type for the purpose of showing that it's thrown by Sally7. I thought current Microsoft recommendations are not to create all kinds of derived exceptions unless they're actually adding meaning on top of the builtin exception types, but again this is something I'm not sure of. Since all these are just exceptions with customized messages only, I doubt the decision to introduce 5 new exception types. Also, while writing this I noticed Sally7SetupException isn't actually ever instantiated, so I guess that should be a static helper class only.

All in all, still very pleased with this PR, also with the latest commits. I might need a bit longer for next review, but I think I'll be able to merge and release on Wednesday at the latest.

[mycroes]

Is this intentionally not deriving from Sally7Exception?

[gfoidl]

No, that's a mistake by me.

Suggested change

	public class Sally7SetupException : Exception
	public class Sally7SetupException : Sally7Exception

Edit:

I guess that should be a static helper class only

Yeah, that makes sense. But I'd like to keep it as exception similar to others. Will make this type internal.

[mycroes]

'violation', not sure if this is a new typo or existing 😄

[gfoidl]

Copy & paste 😃

Suggested change

	=> throw new TpktException("Spec violoation: TPKT reserved is not 0.");
	=> throw new TpktException("Spec violation: TPKT reserved is not 0.");

[mycroes]

I think I would've named this class Sally7CommunicationSetupException, because I think the messages are all related to the Communication Setup messages, which is a specific part of the COTP protocol, IIRC.

[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!