Skip to content
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

Thread safety #29

Closed
Exocomp opened this issue Mar 20, 2020 · 23 comments
Closed

Thread safety #29

Exocomp opened this issue Mar 20, 2020 · 23 comments

Comments

@Exocomp
Copy link

Exocomp commented Mar 20, 2020

On a quad core/4hz machine with .NET Core 3.1 using static vs creating a new instance is about 110 times slower (using encode/decode) testing with 1M integers.

Using a single instance is super fast, so due to performance gains I'd like to use a single instance and share it across threads. So therefore it brings into question thread safety.

I've been testing encode/decode with Parallel.For and I don't see any issues. No locks and ids generated are valid compared to creating new instances.

I'd appreciate if you can confirm thread safety for this library, due to performance gains its worth using a single instance instead of just creating a new instance every time.

@manigandham
Copy link
Collaborator

We use it in millions of calls every minute in a multithreaded app using a static singleton instance. It works fine as-is.

There are lots of other performance improvements that we made for allocations though in our internal fork but you should be fine as far as thread-safety.

@Exocomp
Copy link
Author

Exocomp commented Mar 20, 2020

@manigandham thanks for chiming in, that's reassuring.

For the library maintainers I suggest creating some test cases for thread safety of all the methods just be sure, the performance of a single instance is just too good just to limit this library to always create a new instance. It will only add to the awesomeness of this library.

@ullmark
Copy link
Owner

ullmark commented Mar 20, 2020

@Exocomp there are no maintainers in plural, only me sadly. I would however be very happy if someone would make a PR with those test cases. I recently had a kid and have had rather limited spare time.. I will get there and create those tests, but maybe not as soon as you'd like!

@Exocomp
Copy link
Author

Exocomp commented Mar 20, 2020

@ullmark thank you for creating this library it is super useful! Congrats on your child and I absolutely understand regarding time. I'm super busy also but hopefully sometime in the future we can find some time.

@MrSmoke
Copy link

MrSmoke commented Apr 16, 2021

@manigandham any chance of upstreaming some of those performance updates into this repo?

@manigandham
Copy link
Collaborator

@MrSmoke Sure but it's been years since I worked on our fork and it needs to be redone with the latest language features.

@Daramant has an open PR for better performance right now: #37

@ullmark
Copy link
Owner

ullmark commented Apr 16, 2021

So, the deal is that I don't really use windows much these last years, I'm going to see if I can get it running in macOs and .net core. Otherwise I might be able to remote to a windows computer.

Its great to hear @manigandham, but are there a way to really test thread safety? When this ticket came in I tried googling how to verify that in .net, but couldn't really find anything good?

I would gladly accept someone to contribute to this library and give push/publish access as well, as I've asked earlier.

@manigandham
Copy link
Collaborator

You can test by creating new threads that call the same instance and use Thread.Join to wait for the results. Add in some randomization to the loops and it should be enough for the logic being tested here.

Also the older target frameworks can probably be dropped since they're end of life already, and .NET Framework 4.8 is the last/latest Windows framework which already supports netstandard2.0. This is the latest guidance: https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting

I can help as a contributor, let me know

@ullmark
Copy link
Owner

ullmark commented Apr 16, 2021

@manigandham I'll give you access! 👍

@manigandham
Copy link
Collaborator

@ullmark Got it, thanks. I'll see what I can do in the next few days

@Daramant
Copy link
Contributor

I'd appreciate if you can confirm thread safety for this library, due to performance gains its worth using a single instance instead of just creating a new instance every time.

HashIds store simple data types inside and don't modify it. So, yes, it is thread safe.
Added thread safe test: 5e7d43d

@manigandham
Copy link
Collaborator

hey @ullmark

I merged a bunch of bug fixes and improvements, updated tests, added the github actions workflow, and updated library version to 2.0.0. It should be ready for publishing to nuget. Let me know.

@Daramant
Copy link
Contributor

Hi, @manigandham, final optimizations: #43

@ullmark
Copy link
Owner

ullmark commented Apr 22, 2021

@manigandham let me just briefly explain my thoughts on versioning of this library. This is initially a port of a javascript (or PHP whichever was first) library that now exists in 40+ languages. One of my friends built the Ruby version so I would say the code base is influenced by his port.

I tried to make build it by taking influences from JS to make sure that any changes to the algorithm could be made to .net version as easy as possible, ensuring that a hash created in one language could be used in another given the settings are the same.

At first I followed the versioning of the other libraries, but as where I added features like "long"-support that made no sense in for instance javascript, I started bumping minor version when I added non breaking stuff.

As for these updates I see no reason for updating major version since it doesn't have any breaking changes or big new features?

@manigandham
Copy link
Collaborator

manigandham commented Apr 22, 2021

@ullmark Ah, thanks for the explanation - that makes sense. I'll merge in the new PR by @Daramant which made some more improvements in reducing allocations and will reset the version number to 1.4.0 (since we haven't published it yet).

@ullmark
Copy link
Owner

ullmark commented Apr 22, 2021

@manigandham going to see if I can give you access to publish on nuget as well.

@manigandham
Copy link
Collaborator

manigandham commented Apr 22, 2021

Impressive results from @Daramant

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
RoundtripInts 3.958 us 0.0559 us 0.0523 us 0.0687 - - 576 B
RoundtripLongs 4.546 us 0.0470 us 0.0439 us 0.0687 - - 576 B
RoundtripHex 4.095 us 0.0503 us 0.0470 us 0.1755 - - 1512 B

Build and tests all pass: https://github.com/ullmark/hashids.net/runs/2411647475

@ullmark all set to release version 1.4.0

@manigandham
Copy link
Collaborator

hi @ullmark are you still planning on publishing? anything I can do to help?

@ullmark
Copy link
Owner

ullmark commented May 4, 2021

hi @manigandham. A question; Is there any way of using github for that, seeing a couple of new panels. Otherwise I could add so you have access publishing on nuget, what's your account there?

@ullmark
Copy link
Owner

ullmark commented May 4, 2021

seeing microsoft now owns github right, it should be possible to setup something that deploys directly to nuget? (since I don't really use windows anymore... )

@manigandham
Copy link
Collaborator

There's nothing automatic in github. It supports package hosting but separately from nuget. There are github actions that can push to nuget but I havent set that up before.

My nuget username is https://www.nuget.org/profiles/manigandham

@ullmark
Copy link
Owner

ullmark commented May 4, 2021

@manigandham I created a new Organization called "hashids.net" on nuget where I also invited you as a collaborator. I removed myself as owner of the package and let the organization be the only owner.

I believe you should be able to publish new versions of the package now. Feel free to email me at ullmark@gmail.com in the future if there is something you want to discuss. There is also a "hashids" organization on Github, I can ask them to invite you, but to be honest, not much is happening there 😄

@manigandham
Copy link
Collaborator

Hey everyone - the latest release 1.4.0 is now live: https://www.nuget.org/packages/Hashids.net/1.4.0

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

No branches or pull requests

5 participants