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-2233 Reduce locking for StringInterner lookup and compare methods #7954

Merged

Conversation

finnschiermer
Copy link
Contributor

@finnschiermer finnschiermer commented Aug 6, 2024

What, How & Why?

Reduce locking in the string interner:

  • when using it for comparing strings or for mapping strings to StringIDs.
  • when mapping StringIDs to strings.

Also entirely remove locking from methods where it isn't needed (less important since they are much less frequently used than the methods mentioned above).

Copy link

coveralls-official bot commented Aug 6, 2024

Pull Request Test Coverage Report for Build finn.schiermer-andersen_96

Details

  • 45 of 45 (100.0%) changed or added relevant lines in 2 files are covered.
  • 77 unchanged lines in 16 files lost coverage.
  • Overall coverage increased (+0.005%) to 91.074%

Files with Coverage Reduction New Missed Lines %
src/realm/node_header.hpp 1 63.35%
src/realm/sync/network/http.hpp 1 82.27%
src/realm/sync/network/websocket.cpp 1 72.2%
test/fuzz_tester.hpp 1 57.73%
test/test_index_string.cpp 1 93.33%
test/test_query2.cpp 1 98.75%
src/realm/object-store/shared_realm.cpp 2 91.89%
test/test_lang_bind_helper.cpp 2 93.08%
src/realm/query_engine.cpp 3 92.1%
src/realm/bplustree.cpp 7 72.93%
Totals Coverage Status
Change from base Build jorgen.edelbo_375: 0.005%
Covered Lines: 220199
Relevant Lines: 241781

💛 - Coveralls

@finnschiermer finnschiermer marked this pull request as ready for review August 8, 2024 09:51
@finnschiermer finnschiermer changed the title reduce locking for StringInterner lookup and compare methods RCORE-2233 Reduce locking for StringInterner lookup and compare methods Aug 8, 2024
src/realm/string_interner.cpp Show resolved Hide resolved
//
// Limitations wrt concurrency:
//
// To be used exclusively from Table and in a non-concurrent setting:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have a counter that is increased/decreased when calling the non-threadsafe operation. Then we could have assertions requiring that the counter is 1 inside those functions and 0 in the threadsafe functions below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for such a counter to be correctly shared among threads (and thus be useful for asserting) it would have to be an atomic which carries some overhead. I'm not convinced it is worth it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add it in debug mode only?

@finnschiermer finnschiermer merged commit 80b95d0 into feature/string-compression Aug 9, 2024
46 checks passed
@finnschiermer finnschiermer deleted the fsa/reduce-string-interner-locking branch August 9, 2024 11:26
@nicola-cab
Copy link
Member

nicola-cab commented Aug 12, 2024

@finnschiermer curios to see how the benchmarks look like :-) .. I can run those.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants