Improve performance when resurrecting an object with many backlinks #7218
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 process of first removing the backlink from the tombstone and then creating it again in the live object took a long time. As the backlinks will end up being identical, we can just move the whole structure from the tombstone to the new object and then just replace the link in the linking object without worring about backlinks. This change is only done for link and linklist properties.
jedelbo
force-pushed
the
je/RCORE-1919
branch
from
4ede8fa to
04b746f
Compare
jedelbo
changed the title
Pull Request Test Coverage Report for Build jorgen.edelbo_12
💛 - Coveralls |
jedelbo
changed the title
ironage
approved these changes
Dec 20, 2023
| auto t = m_table->get_opposite_table(col); | ||
| auto c = m_table->get_opposite_column(col); | ||
| auto backlinks = other.get_all_backlinks(col); | ||
|
|
There was a problem hiding this comment.
Clever idea. It leaks the implementation of backlinks storage a bit here, but we have accepted worse in the name of performance.
| @@ -614,7 +612,7 @@ void SyncReplication::set_clear(const CollectionBase& set) | |||
| void SyncReplication::dictionary_update(const CollectionBase& dict, const Mixed& key, const Mixed& value) | |||
| { | |||
| // If link is unresolved, it should not be communicated. | |||
| if (value.is_type(type_Link, type_TypedLink) && value.get<ObjKey>().is_unresolved()) { | |||
| if (value.is_unresolved_link()) { | |||
| @@ -794,4 +806,69 @@ TEST(Links_ManyObjects) | |||
| tr->commit(); | |||
| } | |||
|
|
|||
| TEST(Unresolved_PerformanceLinks) | |||
There was a problem hiding this comment.
It would be ideal to have a micro benchmark for this so we can track it long term to detect regressions, but that's not blocking for me.
Co-authored-by: James Stone <[email protected]>
|
Is there any reason this change might have introduced a performance regression for very large initial sync jobs? Note that the changesets go from taking ~430ms to ~3000ms after a number of them have been processed. If I terminate my app (built with the Realm Swift SDK) and run it again, the changesets again process in about 430ms.
The only change made to the app was updating Realm from 10.44.x to 10.45.2. I did not observe this slowdown during initial bootstrapping on 10.44.x. |
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.
The process of first removing the backlink from the tombstone and then creating it again in the live object took a long time. As the backlinks will end up being identical, we can just move the whole structure from the tombstone to the new object and then just replace the link in the linking object without worring about backlinks.
This change is only done for link and linklist properties.
Fixes #7217
What, How & Why?
☑️ ToDos
bindgen/spec.yml, if public C++ API changed