- Notifications
You must be signed in to change notification settings - Fork1.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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xljiulang commentedNov 19, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
| { | ||
| publicstaticTask<byte[]>ExecuteAsync(thisIMqttRpcClientclient,TimeSpantimeout,stringmethodName,stringpayload,MqttQualityOfServiceLevelqualityOfServiceLevel,IDictionary<string,object>parameters=null) | ||
| [Obsolete("Use the method ExecuteTimeOutAsync instead.")] | ||
| publicstaticasyncTask<byte[]>ExecuteAsync(thisIMqttRpcClientclient,TimeSpantimeout,stringmethodName,stringpayload,MqttQualityOfServiceLevelqualityOfServiceLevel,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.
xljiulang commentedDec 5, 2024
Results of MessageProcessingBenchmark before and after PR.
|
xljiulang commentedDec 7, 2024
The development work of this PR has been completed, but it depends on#2115, otherwise some unit tests will fail. |
| .Build(); | ||
| awaitmqttClient.PublishAsync(applicationMessage,CancellationToken.None); | ||
| awaitmqttClient.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(stringmethodName,byte[]payload,MqttQualityOfServiceLevelqualityOfServiceLevel,IDictionary<string,object>parameters=null,CancellationTokencancellationToken=default); | ||
| { | ||
| Task<ReadOnlySequence<byte>>ExecuteAsync(stringmethodName,ReadOnlySequence<byte>payload,MqttQualityOfServiceLevelqualityOfServiceLevel,IDictionary<string,object>parameters=null,CancellationTokencancellationToken=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?
chkr1011 commentedOct 17, 2025
@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