search

bytedreamer / OSDP.Net

A .NET Core control panel implementation of the Open Supervised Device Protocol(OSDP)
Apache License 2.0
47 stars 19 forks source link

Switched to AsyncKeyedLock #120

Closed MarkCiliaVincenti closed 1 year ago

MarkCiliaVincenti commented 1 year ago

This library will give you better performance and lower memory allocations.

MarkCiliaVincenti commented 1 year ago

@bytedreamer if I'm not mistaken the commit https://github.com/bytedreamer/OSDP.Net/commit/94fc81cb6d8f487b1d62aa50a012066a39063557 is causing CI to fail.

bytedreamer commented 1 year ago

Thanks for contribution request for improving the performance of the library. I will review the code in your project before adding the dependency.

I've submitted fixes for the code analysis warnings. Once it passes, please merge the changes into this pull request.

MarkCiliaVincenti commented 1 year ago

Done @bytedreamer; over to you now.

MarkCiliaVincenti commented 1 year ago

CI is no longer failing now.

bytedreamer commented 1 year ago

I had a chance to review your code changes and the code in the AsyncKeyedLock library. I don't see much of a difference in how memory is allocated between the two methods. I do like how you simplified the creation and usage of the lock dictionary. Is there anything that can be done to improve this project's existing code without add a new dependency?

MarkCiliaVincenti commented 1 year ago

How many combinations of connectionId and address variables will there realistically be? The existing code clears the dictionary only when the Shutdown() method is called and will need to create a SemaphoreSlim object for every combination (which, if there are a lot, will have a number of allocations and also leave leftovers in memory.

Also a cleaner way to do it (with our without my library) would be to create a new class with properties for ConnectionId and Address and either use a custom comparer or implement IEquatable<T> on this class (something like https://github.com/MarkCiliaVincenti/OpenWeatherMap.Cache/blob/master/OpenWeatherMap.Cache/Models/Location.cs) and then use this class as the dictionary key type.

bytedreamer commented 1 year ago

Good point regarding only clearing the dictionary when Shutdown() method is called. Stopping the connection will also remove unneeded dictionary items. I'll have to think about whether to use the hashing method or a custom class for the key.

Thanks for your feedback

MarkCiliaVincenti commented 1 year ago

Are you still considering this PR or shall we close it?

bytedreamer commented 1 year ago

I'm going to hold off on the pull request. Thanks for taking the time to review the code and offer suggestions for improvements.