-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reduce memory allocationt. #2109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
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.
|
…into buffer-reader
{ | ||
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.
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>--> |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
.Build(); | ||
|
||
await mqttClient.PublishAsync(applicationMessage, CancellationToken.None); | ||
await mqttClient.PublishStringAsync( |
There was a problem hiding this comment.
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>--> |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
@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)? |
Now, memory allocation is reduced to 50%.
Job=.NET 8.0 Runtime=.NET 8.0