Reduce memory allocationt. #2109
Conversation
xljiulang
changed the title
|
After refactoring MqttBufferReader, the MQTTnet project can still be reduced to 50% of the pre-pr level, and MQTTnet.AspNetCore can be reduced to 0.58% of the pre-pr level.
|
|
|
||
| public ushort KeepAlivePeriod { get; set; } | ||
|
|
||
| public uint MaximumPacketSize { get; set; } | ||
|
|
||
| public byte[] Password { get; set; } |
There was a problem hiding this comment.
Isn't this also a candidate for ReadOnlyMemory replacement?
There was a problem hiding this comment.
Currently, it cannot be replaced with ReadOnlyMemory because when password is null, it means there is no password and Empty means there is a value. Its logic is consistent with string UserName.
Changing to ReadOnlyMemory affects the related Encode and Decode as well as unit tests. It is recommended to keep it as it is for now because it only affects the connect packet. This change can be completed through a new PR in the future.
|
|
||
| public bool Dup { get; set; } | ||
|
|
||
| public uint MessageExpiryInterval { get; set; } | ||
|
|
||
| public MqttPayloadFormatIndicator PayloadFormatIndicator { get; set; } = MqttPayloadFormatIndicator.Unspecified; | ||
|
|
||
| public ArraySegment<byte> PayloadSegment { set { Payload = new ReadOnlySequence<byte>(value); } } | ||
| public ReadOnlyMemory<byte> PayloadSegment { set { Payload = new ReadOnlySequence<byte>(value); } } |
There was a problem hiding this comment.
I am wondering why we have this strange setter here. Probably a leftover from the last optimization made by @mregen?
There was a problem hiding this comment.
Yes, PayloadSegment is legacy in the low-level MqttPublishPacket, and currently almost only unit test code still uses it.
…into buffer-reader
|
|
||
| namespace MQTTnet.Extensions.Rpc | ||
| { | ||
| public static class MqttRpcClientExtensions | ||
| { | ||
| public static Task<byte[]> ExecuteAsync(this IMqttRpcClient client, TimeSpan timeout, string methodName, string payload, MqttQualityOfServiceLevel qualityOfServiceLevel, IDictionary<string,object> parameters = null) | ||
| [Obsolete("Use the method ExecuteTimeOutAsync instead.")] | ||
| public static async Task<byte[]> ExecuteAsync(this IMqttRpcClient client, TimeSpan timeout, string methodName, string payload, MqttQualityOfServiceLevel qualityOfServiceLevel, IDictionary<string, object> parameters = null) |
There was a problem hiding this comment.
Do we need to keep this method to be compatible with the old version of the API prototype?
After removing these two [Obsolete] methods, we can also overload the ExecuteTimeOutAsync method to the ExecuteAsync method name, and put the TimeSpan timeout parameter in the last parameter, corresponding to the cancellationToken parameter position.
| @@ -14,7 +14,7 @@ | |||
| <NuGetAuditMode>all</NuGetAuditMode> | |||
| <NuGetAudit>true</NuGetAudit> | |||
| <NuGetAuditLevel>low</NuGetAuditLevel> | |||
| <AnalysisLevel>latest-Recommended</AnalysisLevel> | |||
| <!--<AnalysisLevel>latest-Recommended</AnalysisLevel>--> | |||
There was a problem hiding this comment.
try this on .NET 9, if you commented it out because of the 100s of warning messages
<PropertyGroup>
<AnalysisLevel>latest</AnalysisLevel>
<AnalysisMode>recommended</AnalysisMode>
<AnalysisModeStyle>default</AnalysisModeStyle>
</PropertyGroup>There was a problem hiding this comment.
That's 100s of error messages! We can unify this issue in #2120.
There was a problem hiding this comment.
|
Results of MessageProcessingBenchmark before and after PR.
|
|
The development work of this PR has been completed, but it depends on #2115, otherwise some unit tests will fail. |
Now, memory allocation is reduced to 50%.
Job=.NET 8.0 Runtime=.NET 8.0