Skip to content

Conversation

xljiulang
Copy link
Contributor

Now, memory allocation is reduced to 50%.
Job=.NET 8.0 Runtime=.NET 8.0

Method PayloadSize Mean Error StdDev Ratio RatioSD Gen0 Gen1 Allocated Alloc Ratio
Send_1000_Messages_New 8192 8.727 ms 0.1393 ms 0.1368 ms 1.00 0.00 1875.0000 62.5000 8.55 MB 1.00
Send_1000_Messages_Old 8192 18.182 ms 0.1461 ms 0.1220 ms 2.08 0.03 3593.7500 250.0000 16.89 MB 1.98
Send_1000_Messages_New 65536 40.791 ms 0.8041 ms 1.8475 ms 1.00 0.00 11076.9231 3307.6923 63.22 MB 1.00
Send_1000_Messages_Old 65536 32.204 ms 0.6323 ms 0.7997 ms 0.79 0.04 21200.0000 7066.6667 127.05 MB 2.01

@xljiulang xljiulang changed the title Reduce memory allocation of DecodePublishPacket. Reduce memory allocationt. Nov 19, 2024
@xljiulang
Copy link
Contributor Author

xljiulang commented Nov 19, 2024

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.

Method PayloadSize Mean Error StdDev Median Ratio RatioSD Gen0 Allocated Alloc Ratio
AspNetCore_Send_1000_Messages 1024 29.49 ms 0.568 ms 0.697 ms 29.41 ms 1.00 0.00 156.2500 738.92 KB 1.00
MQTTnet_Send_1000_Messages 1024 53.17 ms 1.041 ms 1.022 ms 53.35 ms 1.80 0.05 500.0000 2325.25 KB 3.15
AspNetCore_Send_1000_Messages 8192 32.57 ms 0.634 ms 0.802 ms 32.48 ms 1.00 0.00 125.0000 735.07 KB 1.00
MQTTnet_Send_1000_Messages 8192 54.95 ms 0.537 ms 0.476 ms 55.06 ms 1.69 0.05 2555.5556 9326.39 KB 12.69
AspNetCore_Send_1000_Messages 65536 58.99 ms 1.145 ms 1.406 ms 58.81 ms 1.00 0.00 125.0000 758.25 KB 1.00
MQTTnet_Send_1000_Messages 65536 84.97 ms 4.546 ms 13.405 ms 78.82 ms 1.31 0.10 16714.2857 66269.48 KB 87.40

{
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<NuGetAudit>true</NuGetAudit>
<NuGetAuditLevel>low</NuGetAuditLevel>
<AnalysisLevel>latest-Recommended</AnalysisLevel>
<!--<AnalysisLevel>latest-Recommended</AnalysisLevel>-->
Copy link
Contributor

@mregen mregen Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's 100s of error messages! We can unify this issue in #2120.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xljiulang
Copy link
Contributor Author

Results of MessageProcessingBenchmark before and after PR.


BenchmarkDotNet v0.13.12, Windows 11 (10.0.26100.2314)
12th Gen Intel Core i5-12450H, 1 CPU, 12 logical and 8 physical cores
.NET SDK 9.0.101
  [Host]   : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.10 (8.0.1024.46610), X64 RyuJIT AVX2

Job=.NET 8.0  Runtime=.NET 8.0  

Method PayloadSize Mean Error StdDev Rank Gen0 Gen1 Allocated
Send_1000_Messages_BeforePR 1024 28.66 ms 0.563 ms 1.151 ms 1 781.2500 - 4.48 MB
Send_1000_Messages_BeforePR 4096 31.88 ms 0.237 ms 0.198 ms 2 3187.5000 187.5000 13.27 MB
Send_1000_Messages_BeforePR 8192 33.92 ms 0.208 ms 0.185 ms 3 7000.0000 866.6667 25.05 MB
Method PayloadSize Mean Error StdDev Rank Gen0 Allocated
Send_1000_Messages_AfterPR 1024 29.55 ms 0.229 ms 0.215 ms 1 281.2500 1.75 MB
Send_1000_Messages_AfterPR 4096 31.02 ms 0.207 ms 0.173 ms 2 281.2500 1.76 MB
Send_1000_Messages_AfterPR 8192 32.86 ms 0.172 ms 0.144 ms 3 250.0000 1.84 MB

@xljiulang
Copy link
Contributor Author

The development work of this PR has been completed, but it depends on #2115, otherwise some unit tests will fail.

.Build();

await mqttClient.PublishAsync(applicationMessage, CancellationToken.None);
await mqttClient.PublishStringAsync(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the samples changed? They show how to use the message builder. Please add new samples which are targeting performance relevant ways of publising.

<NuGetAudit>true</NuGetAudit>
<NuGetAuditLevel>low</NuGetAuditLevel>
<AnalysisLevel>latest-Recommended</AnalysisLevel>
<!--<AnalysisLevel>latest-Recommended</AnalysisLevel>-->
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented out?


Task<byte[]> ExecuteAsync(string methodName, byte[] payload, MqttQualityOfServiceLevel qualityOfServiceLevel, IDictionary<string, object> parameters = null, CancellationToken cancellationToken = default);
{
Task<ReadOnlySequence<byte>> ExecuteAsync(string methodName, ReadOnlySequence<byte> payload, MqttQualityOfServiceLevel qualityOfServiceLevel, IDictionary<string, object> parameters = null, CancellationToken cancellationToken = default);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a breaking change to me. Is there a way to support all three methods for now?

@chkr1011
Copy link
Collaborator

@xljiulang I want to merge this PR but it contains too many changes and too many breaking API changes. Is there a way to split this into smaller pieces and avoiding breaking changes (for now, using obsolete attribute etc)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants