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

RCORE-2224 clean up an unused read lock on subscription sets #7949

Merged
merged 3 commits into from
Aug 6, 2024

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Aug 2, 2024

I found this unnecessary version pin when inspecting the version locks while downloading a large bootstrap. Releasing it should improve resource utilization, especially for large sync'd Realms. I'm not sure how long SDKs hold onto the subscriptions for so this change may or may not help in practice.

Fixes #7946

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@ironage ironage self-assigned this Aug 2, 2024
@cla-bot cla-bot bot added the cla: yes label Aug 2, 2024
Query query_a(read_tr->get_table("class_a"));
query_a.greater_equal(fixture.bar_col, int64_t(1));

size_t num_versions_before_subscription = fixture.db->get_number_of_versions();
Copy link
Member

Choose a reason for hiding this comment

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

Windows build failures are because get_number_of_versions is a int64_t, not size_t.

Copy link
Member

@tgoyne tgoyne left a comment

Choose a reason for hiding this comment

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

The Swift SDK always destroys the MutableSubscriptionSet immediately after calling commit() so this won't make a difference there, but other SDKs may be different and seems like a reasonable change regardless.

Copy link

coveralls-official bot commented Aug 2, 2024

Pull Request Test Coverage Report for Build james.stone_583

Details

  • 27 of 27 (100.0%) changed or added relevant lines in 2 files are covered.
  • 42 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.003%) to 91.086%

Files with Coverage Reduction New Missed Lines %
src/realm/index_string.cpp 1 84.63%
src/realm/index_string.hpp 1 93.48%
test/fuzz_tester.hpp 1 57.73%
src/realm/sync/instruction_applier.cpp 3 68.01%
test/test_thread.cpp 3 66.14%
src/realm/sync/noinst/client_impl_base.cpp 7 82.78%
test/fuzz_group.cpp 26 43.54%
Totals Coverage Status
Change from base Build 2545: -0.003%
Covered Lines: 216990
Relevant Lines: 238226

💛 - Coveralls

@ironage ironage merged commit 2c3c3a0 into master Aug 6, 2024
45 checks passed
@ironage ironage deleted the js/subs-version-pin branch August 6, 2024 17:12
@github-actions github-actions bot mentioned this pull request Aug 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate: Write performance when applying data after re-opening the realm
3 participants