-
Notifications
You must be signed in to change notification settings - Fork 165
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-2223 reduce unnecessary table selections in replication #7899
Conversation
Edit: Fixed. Before:
After:
|
59f3be7
to
57f9643
Compare
fix collection notifications after embedded object creations cleanup state, and avoid excessive collection selections fix trace logging
57f9643
to
8ece6c6
Compare
Pull Request Test Coverage Report for Build james.stone_582Details
💛 - Coveralls |
src/realm/replication.hpp
Outdated
ConstTableRef table = coll.get_table(); | ||
ColKey col_key = coll.get_col_key(); | ||
ObjKey obj_key = coll.get_owner_key(); | ||
auto path = coll.get_stable_path(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the values here and passing them to do_select_collection() rather than passing the collection in is a bunch of extra instructions for no benefit and the structure of this means that the compiler can't inline it away.
The idea behind this division is that select_collection() gets inlined into each of the callers so that no function call is needed in the common case of the collection already being selected, but the less common case is outlined rather than being repeated for each called. This is a fairly dubious optimization that we haven't actually measured in the last decade, but it theoretically could be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. I cleaned it up a bit, hopefully this makes it easier for the compiler to make optimizations.
src/realm/replication.hpp
Outdated
{ | ||
if (key != m_selected_obj) { | ||
do_select_obj(key); | ||
bool newly_created = check_for_newly_created_object(key, table); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There doesn't appear to be any obvious reason we have to check this every time rather than caching it and only checking when selecting an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a valid optimization, changed.
This uses the idea in #7734 and takes it a bit further. Although mutations on embedded objects are now ignored, the selections on their tables still made it into the history. In the test download this leads to a history like this:
And with these changes we have the equivalent log:
In this particular test, I observe the following size difference:
Before changes:
Logical file size: 294M
History size: 34.5M
After changes:
Logical file size: 262M
History size: 1.5M
☑️ ToDos
bindgen/spec.yml
, if public C++ API changed