As discussed, this PR removes the MessageAttribute class and replaces its intent with MessageName and MessageNumber properties on the MessageClass.
Profiling showed that for every message sent, reflection was used to get the message number and was a hot path. A performance improvement can be had by replacing the code that requires reflection with simple property access. There is an added benefit that this positions us better for AOT compilation and simplifies the API for messages by not requiring the overlapping behavior of both: MessageAttribute and Message subclass.
Before: when reflection was used to get MessageAttribute for every call to WriteBytes
| Method | Mean | Error | StdDev | Gen0 | Allocated |
|----------- |---------:|----------:|----------:|-------:|----------:|
| WriteBytes | 1.671 us | 0.0326 us | 0.0305 us | 0.1335 | 856 B |
After: when reflection was replaced with properties for every call to WriteBytes
There is a clear improvement in performance and reduction in bytes allocated.
Justification for approach
The use of MessageAttribute on Message classes duplicates behavior. I considered other options like Source Generators to inline the message attribute information or introducing even a new interface for these properties, but ultimately, the abstract base Message class already exists and we don't support any other way to create messages other than inheriting from that class. So, simply adding new properties to it made sense.
Additional considerations
While making these changes, I found we have an analyzer rule (IDE0025) that prevents the use of expression bodies in properties like public override string MessageName => "SSH_MSG_USERAUTH_BANNER"; which I think would make this code much more compact/elegant and is an analyzer rule I think we should consider changing to support.
In a few places, messages exposed static/const internal fields to represent their message number and were also named MessageNumber, causing a name collision. Since the only places I found these internal fields to be used were in unit tests, I removed any existing static MessageNumber in favor of the new instance property and adjusted the tests accordingly. This is also why the new properties are public instead of protected.
The new benchmark needs to be cherry-picked into a branch with/without the changes if you want to run it because I didn't want to replicate the old functionality just for the purposes of the benchmark.
Summary of changes
Excludes the test projects themselves from unit test coverage reports via ExcludeFromTestCoverage attribute. This was simply to make it easier to focus on actual test coverage results.
Removes the MessageAttribute and added the MessageName and MessageNumber abstract properties on the Message base class that every derived class now implements.
Removes the reflection from the Message.WriteBytes method.
As discussed, this PR removes the
MessageAttribute
class and replaces its intent withMessageName
andMessageNumber
properties on theMessageClass
.Profiling showed that for every message sent, reflection was used to get the message number and was a hot path. A performance improvement can be had by replacing the code that requires reflection with simple property access. There is an added benefit that this positions us better for AOT compilation and simplifies the API for messages by not requiring the overlapping behavior of both:
MessageAttribute
andMessage
subclass.Before: when reflection was used to get
MessageAttribute
for every call toWriteBytes
After: when reflection was replaced with properties for every call to
WriteBytes
There is a clear improvement in performance and reduction in bytes allocated.
Justification for approach The use of
MessageAttribute
onMessage
classes duplicates behavior. I considered other options like Source Generators to inline the message attribute information or introducing even a new interface for these properties, but ultimately, the abstract baseMessage
class already exists and we don't support any other way to create messages other than inheriting from that class. So, simply adding new properties to it made sense.Additional considerations While making these changes, I found we have an analyzer rule (IDE0025) that prevents the use of expression bodies in properties like
public override string MessageName => "SSH_MSG_USERAUTH_BANNER";
which I think would make this code much more compact/elegant and is an analyzer rule I think we should consider changing to support.In a few places, messages exposed static/const internal fields to represent their message number and were also named
MessageNumber
, causing a name collision. Since the only places I found these internal fields to be used were in unit tests, I removed any existing staticMessageNumber
in favor of the new instance property and adjusted the tests accordingly. This is also why the new properties arepublic
instead ofprotected
.The new benchmark needs to be cherry-picked into a branch with/without the changes if you want to run it because I didn't want to replicate the old functionality just for the purposes of the benchmark.
Summary of changes
ExcludeFromTestCoverage
attribute. This was simply to make it easier to focus on actual test coverage results.MessageAttribute
and added theMessageName
andMessageNumber
abstract properties on theMessage
base class that every derived class now implements.Message.WriteBytes
method.