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

Add failsafe to RedisLockExtension to resolve #181 #183

Merged
merged 3 commits into from
Aug 18, 2021

Conversation

mgoodfellow
Copy link
Contributor

PR idea for resolving #181

Copy link
Member

@Turnerj Turnerj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@Turnerj Turnerj merged commit 9a6bafa into TurnerSoftware:main Aug 18, 2021
@Turnerj
Copy link
Member

Turnerj commented Aug 18, 2021

Oh, if you were curious, this is what I got benchmarking etc to find the amount the closure affected the code:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
Before fix 10.03 ns 0.617 ns 0.660 ns - - - 96 B
Your original commit 646.30 ns 84.853 ns 97.717 ns - - - 569 B
Without additional closure 522.62 ns 51.334 ns 54.926 ns - - - 481 B
Without additional closure & non-generic TCS 499.49 ns 17.859 ns 18.340 ns - - - 472 B

@mgoodfellow
Copy link
Contributor Author

That is interesting, thanks!

Of course, this isn't really a "hot path" as this code path can only run maximum of once if the lock is already taken. Other code paths won't create the tcs or cts, instead waiting on the one previously created.

The impact of all these closures add up though, I definitely use them liberally as I am a fan of the syntax. I've never really had to focus on raw performance code, so this is all interesting stuff to me!

Thanks for the merge, I'm going to shipping to production in next day or 2 and will monitor!

@Turnerj
Copy link
Member

Turnerj commented Aug 19, 2021

Yeah, definitely not a hot path so I definitely wouldn't bother doing anything too exotic but still nice getting a few small wins.

More important than performance though is that this bug is fixed. Having buggy but fast code isn't particularly helpful.

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