Rework sync config initialization #7857

tgoyne · 2022-06-22T16:10:47Z

Calculate the path from the partition value when a RLMSyncConfiguration is constructed rather than when it's set on a RLMRealmConfiguration, and change RealmSwift.SyncConfiguration to wrap a RLMSyncConfiguration rather than copying the values out. In combination, these changes make it so that we only calculate the path when the configuration is constructed, rather than every time a Realm instance is obtained. The internal initializers for RLMSyncConfiguration have gotten pretty unwieldy over the years so I refactored that a bit, which shouldn't have any effect on the public API.

The newly added perf tests run in ~350ms each on master and <1ms with these changes.

After upgrading to macOS 12.4 building node 13 stopped working for me due it one of the many subcomponents requiring python2, so I switched the baas tests to using the latest LTS version of node.

Calculate the path from the partition value when a RLMSyncConfiguration is constructed rather than when it's set on a RLMRealmConfiguration, and change RealmSwift.SyncConfiguration to wrap a RLMSyncConfiguration rather than copying the values out. In combination, these changes make it so that we only calculate the path when the configuration is constructed, rather than every time a Realm instance is obtained.

It's basically impossible to actually safely assign to it at any point witohut it being atomic, as the reads of it typically happen on background threads.

dianaafanador3

Simplifying the code for RLMSyncConfiguration and get a performance boost out it, is amazing. Great PR

dianaafanador3 · 2022-07-20T15:04:14Z

CHANGELOG.md

@@ -1,7 +1,8 @@
 x.y.z Release notes (yyyy-MM-dd)
 =============================================================
 ### Enhancements
-* None.
+* Greatly improve the performance of obtaining cached Realm instances in Swift


Isn't this enhancement coming from a user issue, could be valuable to tag this here?

dianaafanador3 · 2022-07-20T15:05:26Z

Realm/ObjectServerTests/RLMFlexibleSyncServerTests.mm

@@ -953,7 +953,7 @@ - (void)testFlexibleSyncInitialSubscriptionAwait {
        CHECK_COUNT(11, Person, realm);
        [ex fulfill];
    }];
-    [self waitForExpectationsWithTimeout:10.0 handler:nil];
+    [self waitForExpectationsWithTimeout:30.0 handler:nil];


Any reason for extending the timeout times in most of the server tests and the app creation?, isn't this enhancement supposed to make opening the realm faster

These tests have been failing pretty consistently with timeouts due to the server being slow. All of the timeouts I bumped were ones which failed at least occasionally when using Xcode's "run tests repeatably until failure".

dianaafanador3 · 2022-07-20T15:10:17Z

Realm/ObjectServerTests/RLMObjectServerTests.mm

@@ -2624,6 +2624,13 @@ - (void)testWatchWithMatchFilterAsync {

 - (NSArray<RLMObjectId *> *)insertDogDocuments:(RLMMongoCollection *)collection {
    __block NSArray<RLMObjectId *> *objectIds;
+    XCTestExpectation *ex = [self expectationWithDescription:@"delete existing documents"];


Aren't we cleaning all documents on every tearDown, why duplicating it here?

No, we don't delete anything on tearDown. The sync tests use a separate partition for each test so that they don't see each others data, but these tests are accessing the collection directly.

does deleting all documents before inserting obscure what this function is intended for? Should the calling function just call something like deleteAllDogDocuments and then call insertDogDocuments?

We never want to call them separately, but I guess the name should be prepareDogDocuments or something. The thing being fixed here is that without deleting old documents the tests only work when run in a specific order and break when run in other orders due to the missing cleanup.

Sure, lets change the name to avoid confusion in the future

dianaafanador3 · 2022-07-20T15:35:55Z

Realm/RLMRealmConfiguration.mm

@@ -179,7 +179,9 @@ - (void)setInMemoryIdentifier:(NSString *)inMemoryIdentifier {

 - (void)setSeedFilePath:(NSURL *)seedFilePath {
    _seedFilePath = seedFilePath;
-    _config.in_memory = false;
+    if (_seedFilePath) {


Isn't this redundant, _seedFilePath cannot be null because we just set it.

This change is making config.seedFilePath = nil valid.

dianaafanador3 · 2022-07-20T16:03:35Z

Realm/RLMSyncConfiguration.mm


-        self.customFileURL = customFileURL;
-        return self;
+- (instancetype)initWithUser:(RLMUser *)user


Very good job simplifying the RLMSyncConfiguration initialisation

dianaafanador3 · 2022-07-20T16:14:22Z

RealmSwift/Tests/PerformanceTests.swift

+        _ = realm.configuration
+        deleteServerFiles()
+    }
+


I see most of this test are PBS, I know how we build the sync configuration is pretty similar for for FLX and PBS, but we do obtain the path differently for each type of sync configuration, it could be nice to have a test using a flexible sync configuration giving that FLX is our go to sync configuration in the future.

I agree with Diana

I've updated it to test both flx and pbs and added comments for the less obvious bits in the test.

dianaafanador3 · 2022-07-21T10:31:57Z

RealmSwift/Tests/PerformanceTests.swift

+                }
+            }
+        }
+        _ = realm.configuration


Why do we need to read the configuration at the end of this test, is this adding any value to the test?

Swift (sometimes) drops references to objects after the last time they are used rather than at the end of the scope, so without this the realm may be deallocated before the actual test runs.

dianaafanador3 · 2022-07-21T10:48:40Z

Realm/RLMUser.mm

-                                                 clientResetMode:clientResetMode
-                                               notifyBeforeReset:beforeResetBlock
-                                                notifyAfterReset:afterResetBlock];
+                                                  partitionValue:partitionValue];


Changing the stop policy for this particular method, wouldn't be a breaking change?, I don't know that much but RLMSyncStopPolicyImmediately maybe makes sense during a client reset

SessionStopPolicy isn't part of the public API, and one of the initializers using a different stop policy from the rest was just a bug. This isn't used during a client reset; it's just a convenience initializer for setting the callbacks while initializing a configuration, and that clearly shouldn't result in an unrelated property being set differently.

dianaafanador3 · 2022-07-21T11:12:27Z

Realm/RLMRealmConfiguration.mm

@@ -373,7 +368,7 @@ - (RLMSyncConfiguration *)syncConfiguration {
    if (!_config.sync_config) {
        return nil;
    }
-    return [[RLMSyncConfiguration alloc] initWithRawConfig:*_config.sync_config];


Do we still need to initialise the RLMSyncConfiguration every time we are reading the syncConfiguration, doesn't RLMSyncConfiguration holds a reference to the SyncConfig from core

RLMRealmConfiguration does not hold onto a RLMSyncConfiguration instance, so we have to create the obj-c wrapper on each access. IIRC retaining it has awkward reference cycle issues.

tgoyne self-assigned this Jun 22, 2022

cla-bot bot added the cla: yes label Jun 22, 2022

tgoyne mentioned this pull request Jun 22, 2022

Big performance issues with v10.24.2 and Synced Realm #7743

Closed

tgoyne force-pushed the tg/sync-config branch 2 times, most recently from b4bbe62 to af0ab70 Compare June 24, 2022 17:24

tgoyne force-pushed the tg/sync-config branch 3 times, most recently from 8719371 to b985477 Compare July 1, 2022 01:53

dianaafanador3 mentioned this pull request Jul 6, 2022

memory leak ios #7708

Closed

tgoyne added 8 commits July 6, 2022 13:19

Switch to using node 16 for tests

efe75b8

Update SwiftUI sync tests for changes to the flexible sync API

960115a

Make RLMSyncManager.errorHandler atomic

1a0f70e

It's basically impossible to actually safely assign to it at any point witohut it being atomic, as the reads of it typically happen on background threads.

Lengthen some overly short timeouts in tests

1d520f4

Fix a test which did not test what it intended to test

be9bcae

Add some missing test cleanup

9fad9d4

Reduce the server polling period further and disable some unused things

02dce57

tgoyne force-pushed the tg/sync-config branch from b985477 to 02dce57 Compare July 6, 2022 20:19

tgoyne requested review from dianaafanador3 and jsflax July 7, 2022 03:35

dianaafanador3 reviewed Jul 21, 2022

View reviewed changes

tgoyne requested a review from leemaguire July 26, 2022 15:35

Test both flexible and partition-based sync Realm init

9e204e6

leemaguire approved these changes Jul 26, 2022

View reviewed changes

tgoyne merged commit 878e7f6 into master Jul 27, 2022

tgoyne deleted the tg/sync-config branch July 27, 2022 04:00

github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework sync config initialization #7857

Rework sync config initialization #7857

tgoyne commented Jun 22, 2022

dianaafanador3 left a comment

dianaafanador3 Jul 20, 2022

dianaafanador3 Jul 20, 2022

tgoyne Jul 25, 2022

dianaafanador3 Jul 20, 2022

tgoyne Jul 25, 2022

leemaguire Jul 26, 2022

tgoyne Jul 26, 2022

leemaguire Jul 26, 2022

dianaafanador3 Jul 20, 2022

tgoyne Jul 25, 2022

dianaafanador3 Jul 20, 2022

dianaafanador3 Jul 20, 2022

leemaguire Jul 26, 2022

tgoyne Jul 26, 2022

dianaafanador3 Jul 21, 2022

tgoyne Jul 25, 2022

dianaafanador3 Jul 21, 2022

tgoyne Jul 25, 2022

dianaafanador3 Jul 21, 2022

tgoyne Jul 25, 2022

Rework sync config initialization #7857

Rework sync config initialization #7857

Conversation

tgoyne commented Jun 22, 2022

dianaafanador3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment