Internal sync #21
Merged
Internal sync #21
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
Signed-off-by: Joe Runde <[email protected]> Co-authored-by: Nick Hill <[email protected]> Co-authored-by: TRAVIS JOHNSON <[email protected]>
- grpc 1.45.0 - guava 31.1 - protobuf 3.19.4 - jackson-databind 2.13.2 - log4j 2.17.2 Signed-off-by: Nick Hill <[email protected]>
Motivation We had hoped that the notorious cascading eviction bug was fixed recently but though still infrequent it was observed once again in the wild. Upon close analysis of these latest occurrences in production logs it was possible to determine the source of the remaining race condition. The problem basically comes down to the fact that we attempt to "immediately" adjust the unload buffer accounting at the point entries are evicted from the cache, but this was still not done atomically. So with the right confluence of events between different threads, it was possible for a model loading thread to incorrectly think there's sufficient space to proceed rather than waiting for other unloads to complete. This in turn results in the over-committed state that causes the eviction cascade. Modifications - Unify the cache's eviction lock with the separate UNLOAD_BUF lock, and move the eviction callbacks within the locked context. - Eliminate the separate eviction thread and instead perform the eviction processing inline (including related cache accounting updates). - Add unit tests to exercise eviction / unload buffer behaviour and to trigger an eviction cascade prior to this fix. Result Hopefully no more cascading evictions possible. Signed-off-by: Nick Hill <[email protected]>
|
/hold |
pvaneck
approved these changes
Mar 10, 2022
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhill, pvaneck The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
This is really expected to be the final sync of IBM-internal fixes, future PRs will be made directly against this repo.