-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: RTU communication message Log #117
base: master
Are you sure you want to change the base?
Conversation
zh3305
commented
May 28, 2024
Hi, thanks for your PR. I understand the reason why you would like to see it implemented but I have some concerns regarding performance. Specifically the following LOCs will put a large burden on the garbage collector:
I am afraid that I cannot merge your PR with these many allocations as I also have no immediate solution to avoid these allocations. The only workaround would be to put preprocessor directives around the allocation-heavy logging code like this
This would mean the code will not become part of the Nuget package but would be available when cloning this repo and directly referencing it. I am not sure if this would be a practical solution, maybe you can share your thoughts on this. Thanks! |
Thank you for your feedback. I appreciate your concerns regarding performance. With regard to log performance, I propose implementing a toggle switch named For the performance issue related to converting bytes to hexadecimal, I have referenced an optimized solution found at https://gist.github.com/antoninkriz/915364de7f264dd14a572936abd5228b. This approach aims to minimize allocations and improve conversion speed.For non-.NET 5 environments, in this Stack Overflow post: https://stackoverflow.com/a/24343727/3161322. The benchmark results indicate significant performance gains, especially in terms of allocation reduction. Here is a summary of the benchmark results:
Thank you for your valuable input! |