Remove spin lock from credentials #191
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Trying a model where slots of credentials exist, and updates move across the slots. Mutation detection is attempted via versioning of the credential changes.
Added debug status for the test to emit. Just switched to a single versioned credentials.
Added more comments to the creds provider to make the intention clearer. Simplified the usage of the object fields as part of removing an earlier design. Modified the tests to use the logger instead of std::cout
sayantacC
approved these changes
Mar 21, 2018
| std::lock_guard<std::mutex> lock(update_mutex_); | ||
|
|
||
| // | ||
| // The ordering stores here is important. current_.updating_, and current_.version_ are atomics. |
| } | ||
| update_step(debug_stats.success_); | ||
| return result; | ||
| } while (true); |
There was a problem hiding this comment.
Is an infinite loop the right thing to do when the credential setter does not it correctly ? Or should we error out after some time ?
There was a problem hiding this comment.
Went back through my testing, and determined that devolving to the lock would provide similar performance. Made the changes for that now.
| update_step(debug_stats.update_before_load_, debug_stats.retried_); | ||
| continue; | ||
| } | ||
| std::uint64_t starting_version = current_.version_.load(); |
There was a problem hiding this comment.
should we be consistent in our use of atomic. If we use the atomic<T>::operator T then we should use it everywhere instead of the equivalent .load method unless we really want to specify memory order.
The credentials provided will now devolve to a full lock when an optimistic read fails. On my limited testing this happened ~0.007% of the times. Corrected some comments, and removed explicit uses of load()
Renamed the optimistic_read to try_optimistic_read to indicate that will update the argument passed to it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removed the spin lock from the credentials class. The spin lock was really expensive and problematic performance wise.
The new approach uses an optimistic read via an interlocked change process to allow the get method to detect a change and retry the load of the credentials.