Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to core 14 #8496

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Upgrade to core 14 #8496

merged 1 commit into from
Mar 21, 2024

Conversation

tgoyne
Copy link
Member

@tgoyne tgoyne commented Feb 23, 2024

All tests now pass. Still need to fix some performance regressions.

@tgoyne tgoyne self-assigned this Feb 23, 2024
@cla-bot cla-bot bot added the cla: yes label Feb 23, 2024
@tgoyne tgoyne force-pushed the tg/core-14 branch 4 times, most recently from 0b875df to 4db3061 Compare March 1, 2024 23:19
@tgoyne tgoyne force-pushed the tg/core-14 branch 2 times, most recently from a9f61dc to 9ece953 Compare March 8, 2024 22:22
@tgoyne tgoyne force-pushed the tg/core-14 branch 2 times, most recently from c4ca77e to 7d8abf2 Compare March 14, 2024 01:47
@tgoyne tgoyne added the no-jira-ticket Skip checking the PR title for Jira reference label Mar 14, 2024
@tgoyne tgoyne marked this pull request as ready for review March 14, 2024 02:53
@tgoyne
Copy link
Member Author

tgoyne commented Mar 14, 2024

Needs another core release to pick up the final perf regression fix before it can actually be merged, but the code changes here are now reviewable.

@tgoyne tgoyne force-pushed the tg/core-14 branch 2 times, most recently from db3105d to 1fac1bf Compare March 15, 2024 21:57
@tgoyne
Copy link
Member Author

tgoyne commented Mar 15, 2024

Core 14.3.0 should be good for an actual release.

Copy link
Collaborator

@dianaafanador3 dianaafanador3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!, Just some minor questions.
Also, changes to the performance tests number of iterations is because we want to evaluate performance for bigger operations?

@@ -1557,7 +1556,7 @@ struct SectionedResultsTestDataAnyRealmValue: SectionedResultsTestData {
}

static func orderedKeys(ascending: Bool) -> [String] {
return ["alphanumeric", "data"]
return ascending ? ["alphanumeric", "data"] : ["data", "alphanumeric"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes to the sorting menas this was not working before or not been used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sort order used to be that the values were ordered ["alphanumeric", "data", "data", "alphanumeric"]. The order of the sections is determined by the order of the values and not by sorting the keys, so it ended up being the same regardless of what order the objects were sorted.

for _ in 0..<500 {
_ = realm.objects(ModernSwiftStringProjection.self)
}
measure(times: 5) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to reduce the number of iterations to measure?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test previously wasn't actually doing much of anything and the changed tests is doing a lot more.

}
[realm commitWriteTransaction];

[self startMeasuring];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we measuring exactly on this performance test?, The time committing changes to the 5 objects?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time to produce the change notification, which will stop the run loop once it's delivered.

// coercion and checking both
if constexpr (is_any_v<C, Columns<Mixed>, Columns<Lst<Mixed>>, Columns<Set<Mixed>>, Columns<Dictionary>>) {
Mixed m = value;
if (!m.is_null()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this changes are we replacing some core removed implementation?, to maintain compatibility. Does that mean this is no longer needed, and we can remove this in major a version?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this can go away with a major version bump. I'll add a comment noting that.

@tgoyne tgoyne merged commit 49c5912 into master Mar 21, 2024
131 of 133 checks passed
@tgoyne tgoyne deleted the tg/core-14 branch March 21, 2024 18:59
nirinchev added a commit that referenced this pull request Apr 10, 2024
* master:
  RCOCOA-2305: Update base url to point to services.cloud.mongodb.com (#8537)
  Update docs URL (#8538)
  RCOCOA-2310: Fix a crash when receiving 401/403 when opening a watch stream (#8536)
  Add core version to build-binaries workflow (#8525)
  Add an empty changelog section
  Upgrade to core 14.4.1 (#8526)
  Release v10.49.0
  Upgrade to core 14 (#8496)
  RCOCOA-2308: Add GHA workflow for core prebuilds (#8517)
  Release 10.48.1
  Add NSPrivacyAccessedAPICategoryDiskSpace to the privacy manifest (#8511)
  🔄 Synced file(s) with realm/ci-actions (#8520)
  Add build-binaries workflow (#8515)
  Only commit the changelog in the add empty changelog release step (#8505)
  Release 10.48.0
  Introduce `_customRealmProperties()` to all Realm Objects to allow for client-generated RLMProperties (via Swift Macros) (#8490)
  Update packaging for Xcode 15.3 (#8502)

# Conflicts:
#	Realm/RLMAsyncTask.mm
#	Realm/RLMSyncSession.mm
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes no-jira-ticket Skip checking the PR title for Jira reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants