RobThree / IdGen

Twitter Snowflake-alike ID generator for .Net
MIT License
1.19k stars 148 forks source link

Feature: CreateId that accepts generator ID as argument #30

Closed AachoLoya closed 3 years ago

AachoLoya commented 3 years ago

We have a use case where we need to specify a custom generator ID for each request since we encode some other info in each ID. Since the library only generates IDs with three parts epoch + generator ID + sequence, our only option is to use custom generator ID - that we create using our logic.

https://github.com/RobThree/IdGen/blob/master/IdGen/IdGenerator.cs#L100

Can you create a method 'long CreateIdusingGenId(int generatorId)' and then update CreateIdImpl signature to have generator id as parameter (default value -1), In case of CreateId, pass -1 as generator Id and it will use member _generatorid value, otherwise it will use the provided one.

RobThree commented 3 years ago

Why not use the specific generator to generate an Id? Just create an instance of each generator (I'd keep them around, maybe in an array or dictionary or...) and call CreateId() on the generator that you want?

AachoLoya commented 3 years ago

I did think by doing that, but our logic is a bit complex so we'd have to maintain an array of 16+ generators. Not unreasonable, but thought if we could just pass generator Id in createId, it'll be a lot cleaner code.

AachoLoya commented 3 years ago

Another feature would be to replace the lock with SemaphoreSlim(1,1) - which allows WaitAsync, and make the method CreateId async, so that it can be used in asp.net without affecting performance.

RobThree commented 3 years ago

I did think by doing that, but our logic is a bit complex so we'd have to maintain an array of 16+ generators. Not unreasonable, but thought if we could just pass generator Id in createId, it'll be a lot cleaner code.

It may be cleaner code in your case but I think it clouds the code for the general usecase. You're using IdGen's generator ID "creatively" to 'bend' it to encode some other info into the Id; I don't think that's the case we should design for.

Another feature would be to replace the lock with SemaphoreSlim(1,1) - which allows WaitAsync, and make the method CreateId async, so that it can be used in asp.net without affecting performance.

There's nothing async about the method; and making it async doesn't magically improve performance somehow. The method is fast (there's probably some room for improvement) and I don't see how making it async would improve matters. It's like asking for a Guid.NewGuidAsync().

AachoLoya commented 3 years ago

It may be cleaner code in your case but I think it clouds the code for the general usecase. You're using IdGen's generator ID "creatively" to 'bend' it to encode some other info into the Id; I don't think that's the case we should design for.

If you look at other derivates of Snowflake , e.g. Instagram, discord, they all took the format but adopted it, mainly the middle part (generator Id) to their own usage. It's upto you if the change is worth it or not. I decided to just made a local copy of this project and modified the code and locking to fit our usage.

There's nothing async about the method; and making it async doesn't magically improve performance somehow. The method is fast (there's probably some room for improvement) and I don't see how making it async would improve matters. It's like asking for a Guid.NewGuidAsync().

In asp.net any thread blocking call is strongly discouraged, which is exactly what 'lock' does. The reason being blocking a thread takes away a thread that could have processed another request. That's the reason why SemaphoreSlim was introduced, so you can await on the semaphore by calling WaitAsync, without actually blocking a thread.

These are just my suggestions, still the project was really helpful for us. Thanks!

RobThree commented 3 years ago

The reason being blocking a thread takes away a thread that could have processed another request.

For long-running tasks or being memory-, cache- or I/O bound. But IdGen is CPU bound and not long running and the locks aren't for any long running tasks. If you have changed this in your local copy, could you benchmark it compared to this version and share your results?