Improve performance with very large numbers of notifiers #7424
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
std::unordered_map is pretty slow and TableKeys are *almost* dense so we can use a vector instead. This speeds up almost--noop notifications in obj-c by ~20%.
With large notifier counts the lock contention on CollectionNotifier::m_realm becomes a very significant overhead, with unlock() taking upwards of 50% of the total runtime. This is only a problem when the notifiers run on the background thread and the target thread forces a refresh, as there's otherwise no contention. Using an atomic variable to avoid the mutex solves the problem entirely.
ironage
reviewed
Mar 6, 2024
| std::vector<LinkInfo> complete_mapping; | ||
| complete_mapping.resize(group->size()); | ||
| auto get_mapping = [&](TableKey key) -> LinkInfo& { | ||
| size_t ndx = Group::key2ndx(key); |
There was a problem hiding this comment.
It looks like table keys will be recycled after 100 table removals. I think that would lead to an already initialized LinkInfo being returned from here. Even if not, it seems like a risk that we are using implementation details of a TableKey (though we have done worse in the name of performance). Could we get similar performance improvement by using our util::FlatMap?
There was a problem hiding this comment.
This is a temporary data structure that doesn't outlive this function, and the state of the transaction it's reading from can't change while it exists.
There was a problem hiding this comment.
Oh, you're right. All good then, thanks! 👍
ironage
approved these changes
Mar 6, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
With large notifier counts the lock contention on CollectionNotifier::m_realm becomes a very significant overhead, with unlock() taking upwards of 50% of the total runtime. This is only a problem when the notifiers run on the background thread and the target thread forces a refresh, as there's otherwise no contention. Using an atomic variable to avoid the mutex solves the problem entirely.
std::unordered_map is pretty slow and TableKeys are almost dense so we can use a vector instead. This speeds up almost-noop notifications where gathering the related table information is the bulk of the runtime in obj-c by~20%.
Together these speed up one of the obj-c benchmarks from 2.959 seconds to .765 seconds. The newly added object-store benchmark is less dramatic but still goes from 86ms to 46ms.