remove unnecessary callback use for ArrayWithFind::find_action() #7095
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build james.stone_406
💛 - Coveralls |
|
I did something similar in 9a33838. I think your approach is better, but |
ironage
force-pushed
the
js/skunk-2023-October
branch
from
af5bc36 to
badac29
Compare
|
Thanks for that feedback Thomas, I've removed |
tgoyne
approved these changes
Oct 30, 2023
src/realm/query_conditions_tpl.hpp
Outdated
| value = m_source_column->get_any(index); | ||
| } | ||
| else { | ||
| static_cast<void>(m_source_column); |
There was a problem hiding this comment.
Shouldn't be any reason to need this. If it was an if constexpr guarding the use of m_source_column you might get some spurious warnings but here the member variable is always used.
src/realm/array_with_find.hpp
Outdated
|
|
||
| cond: One of Equal, NotEqual, Greater, etc. classes | ||
| Action: One of act_ReturnFirst, act_FindAll, act_Max, act_CallbackIdx, etc, constants | ||
| Callback: Optional function to call for each search result. Will be called if action == act_CallbackIdx | ||
| Action: One of act_ReturnFirst, act_FindAll, act_Max, etc, constants |
There was a problem hiding this comment.
The Action template parameter hasn't existed for a while.
nicola-cab
approved these changes
Oct 31, 2023
|
CI failures appear unrelated. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The query system supported two ways of dealing with matches: using QueryStateBase::match() and using a custom callback.
The callback was only used in one place, and I replaced it by caching the results array leaf inside our existing QueryStateBase handler class.
There is no noticeable performance drop for the affected query case that was using the callback. Here's the newly added benchmark compared to master showing no significant change for this case:
The benefit of doing this is that we can delay fetching the values until the
match()function which may not even need the values at all, such as when counting the results. This should have some minimal performance improvement when counting results with many matches.In addition to the code clean up, this also removes a layer of templating throughout the query system which appears to remove about 100K of binary size off our static library (YMMV).
☑️ ToDos