bwmarrin / snowflake

A simple to use Go (golang) package to generate or parse Twitter snowflake IDs
BSD 2-Clause "Simplified" License
2.98k stars 371 forks source link

Use Monotonic clock #18

Closed nishaad78 closed 5 years ago

nishaad78 commented 5 years ago

Starting from Go 1.9, the standard time package transparently uses Monotonic Clocks when available. Let's use that for generating ids to safeguard against wall clock backwards movement which could be caused by time drifts or leap seconds.

This PR addresses the aforementioned issue which may lead to collisions.

bwmarrin commented 5 years ago

I love this :)

I'm wondering if you would mind helping me and changing it slightly so to avoid breaking the API?

I'm thinking that maybe we can leave Epoch as an int64. Create an epoch variable in the node struct that is the right type, then in the NewNode func take the int64 epoch value and apply it to the node.epoch value. Then on all Generate() code you can reference the node.epoch value. This also lets us not add an additional global value curTime.

Then the Time func will take a bit of changing but it's a deprecated func that I intend to replace as a member func off of the node struct.

Does that make sense?

nishaad78 commented 5 years ago

Yes, it makes sense. I also like this better as it hides this ugly hack: curTime.Add(epoch.Sub(curTime))

bwmarrin commented 5 years ago

Great!

bwmarrin commented 5 years ago

Just to add this in here, as an interesting note.

I ran the benchmarks before and after this change and there's some interesting changes.

Before:

BenchmarkGenerate-12                     5000000               243 ns/op               0 B/op          0 allocs/op
BenchmarkGenerateMaxSequence-12         20000000               113 ns/op               0 B/op          0 allocs/op

After:

BenchmarkGenerate-12                     5000000               319 ns/op               0 B/op          0 allocs/op
BenchmarkGenerateMaxSequence-12         20000000                76.3 ns/op             0 B/op          0 allocs/op

I'm amazed that BenchmarkGenerateMaxSequence dropped so much while BenchmarkGenerate went up. I'll dig into this more later - or you're welcome to explore it as well. Once thing here is now BenchmarkGenerate takes over 243ns/op which means it cannot (on my computer) create the 4096 ID's per ms that the standard snowflake format supports.

nishaad78 commented 5 years ago

Yes, I think the comparison logic in the if statement with my change was less efficient. I've update it to how it was before and here are the results, much better now :)

Before (v0.1.0):

BenchmarkGenerate-4                      5000000               244 ns/op               0 B/op          0 allocs/op
BenchmarkGenerateMaxSequence-4          20000000               113 ns/op               0 B/op          0 allocs/op

After (fix in #19) :

BenchmarkGenerate-4                      5000000               243 ns/op               0 B/op          0 allocs/op
BenchmarkGenerateMaxSequence-4          20000000                66.6 ns/op             0 B/op          0 allocs/op

It is faster in my implementation in BenchmarkGenerateMaxSequence because of the optimized time.Since func. After (fix in #19, using time.Now().Sub(epoch)):

BenchmarkGenerate-4                      5000000               243 ns/op               0 B/op          0 allocs/op
BenchmarkGenerateMaxSequence-4          20000000               111 ns/op               0 B/op          0 allocs/op
nishaad78 commented 5 years ago

Please check pull request #19

bwmarrin commented 5 years ago

that's awesome. Thanks!