[SharedCache] Reduce lock contention in SharedCache::LoadSectionAtAddress
#6202
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
The existing view-specific state was stored in several global unordered maps. Many of these were accessed without locking, including `viewSpecificMutexes`, which is racy in the face of multiple threads. View-specific state is stored in a new heap-allocated `ViewSpecificState` struct that is reference counted via `std::shared_ptr`. A static map holds a `std::weak_ptr` to each view-specific state, keyed by session id. `SharedCache` retrieves its view-specific state during its constructor. Since `ViewSpecificState` is reference counted it will naturally be deallocated when the last `SharedCache` instance that references it goes away. Its corresponding entry will remain in the static map, though since it only holds a `std::weak_ptr` rather than any state it will not use much memory. The next time view-specific state is retrieved any expired entries will be removed from the map.
They're surprisingly expensive to look up.
…dress` When loading a number of sections, for instance when an image is loaded, many of the analysis threads bottleneck in `SharedCache::LoadSectionAtAddress` trying to acquire the `viewOperationsThatInfluenceMetadataMutex` lock. This is a very expensive lock to try and acquire as it is used in a lot of places and held for long durations. This commit improves performance in `SharedCache::LoadSectionAtAddress` by not acquiring the `viewOperationsThatInfluenceMetadataMutex` lock until absolutely necessary. I.e. when something actually needs to be loaded. Often what happens is many threads try to load the same sections but ending up queuing up to do this one at a time. The commit adds per memory region locking so that threads only block waiting for the memory region they require to be loaded. In most cases though, if the region is already loaded they won't wait at all because no lock is required to determine if this is the case.
bdash
reviewed
Nov 26, 2024
|
|
||
| // The region appears not to be loaded. Acquire the loading lock, re-check | ||
| // that it hasn't been loaded and if it still hasn't then actually load it. | ||
| std::unique_lock<std::mutex> memoryRegionLoadingLockslock(ViewSpecificStateForView(m_dscView)->memoryRegionLoadingMutexesMutex); |
There was a problem hiding this comment.
The view-specific state can be found in m_viewSpecificState rather than using ViewSpecificStateForView(…) to look it up in the global hash table.
…okup This is just correcting a silly mistake
plafosse
added
the
File Format: SharedCache
|
Thank you for the PR! We're looking into getting this merged over the next couple of weeks |
|
I'm currently doing some rather sweeping changes to the code in regards to the usage of locking. I'm not 100% confident in some of the changes I've made in this PR and I actually think I have a better version in the works. I've got it so I can do bulk parallelised loading of images which can dramatically bring down load times to very acceptable levels. Although it is somewhat reliant on #6271 being fixed and the UI freezes if many changes are made that quickly so you have to do it in the background. |
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.
When loading a number of sections, for instance when an image is loaded, many of the analysis threads bottleneck in
SharedCache::LoadSectionAtAddresstrying to acquire theviewOperationsThatInfluenceMetadataMutexlock. This is a very expensive lock to try and acquire as it is used in a lot of places and held for long durations.This commit improves performance in
SharedCache::LoadSectionAtAddressby not acquiring theviewOperationsThatInfluenceMetadataMutexlock until absolutely necessary, i.e. when something actually needs to be loaded. Often what happens is many threads try to load the same sections but ending up queuing up to do this one at a time. The commit adds per memory region locking so that threads only block waiting for the memory region they require to be loaded. In most cases though, if the region is already loaded they won't wait at all because no lock is required to determine if this is the case.Note: this PR is built on @bdash's PR #6196 due to the fixes and improvements in view-specific state, as this commit adds view-specific mutexes. I thought it would be prudent to make use of those changes.
To be honest I'm not 100% sure about the use of a map of mutexes to provide per memory region locking. It just feels a bit excessive but it works and the memory consumption of it won't be anything significant.