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

Use Monotonic clock #18

Merged
merged 7 commits into from
Apr 11, 2019
Merged

Use Monotonic clock #18

merged 7 commits into from
Apr 11, 2019

Conversation

nishaad78
Copy link
Contributor

@nishaad78 nishaad78 commented Mar 15, 2019

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
Copy link
Owner

bwmarrin commented Apr 11, 2019

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
Copy link
Contributor Author

nishaad78 commented Apr 11, 2019

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

@bwmarrin
Copy link
Owner

Great!

@bwmarrin bwmarrin closed this Apr 11, 2019
@bwmarrin bwmarrin reopened this Apr 11, 2019
@bwmarrin bwmarrin merged commit 652d4f1 into bwmarrin:master Apr 11, 2019
@bwmarrin
Copy link
Owner

bwmarrin commented Apr 11, 2019

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
Copy link
Contributor Author

nishaad78 commented Apr 11, 2019

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
Copy link
Contributor Author

Please check pull request #19

@bwmarrin
Copy link
Owner

that's awesome. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants