Delegating the matches in PointRangeQuery weight to relate method #13599
Conversation
iverase
left a comment
iverase
left a comment
There was a problem hiding this comment.
I would expect this change to result in a slow down for this type of queries.
You are proposing to replace the current implementation with a slower one that computes the relationship between a range and the query. Note that this method is the hottest in the execution path.
Have you done in performance benchmark of the change?
@iverase i'm struggling to see how this could be worse, it looks like we are simply delegating from matches to relate to simplify the logic, relate should return the cell attribute after comparing the values quickly. what am I missing? |
|
If we look at the current implementation of matches and relates, they both iterate over the dimensions and they both check if the dimension is disjoint. If that is true, then they bail out. The difference is when the dimensions are not disjoint because the relate method executes another check to compute if the dimensions crosses or is fully inside the range: This is the extra check your change is adding which IMHO is not a good idea. |
|
@iverase - Adding the benchmark results below: |
|
Also, adding the cpu/memory profile below: BASELINE: CANDIDATE: |
|
Memory profile: CANDIDATE: |
|
Overall, I am not seeing any performance regression from the benchmark or memory/cpu profiles. Please let me know, in case I am missing something. |
|
I am afraid those benchmarks hardly exercise the change you are proposing so they will not show anything. |
Do you have anything specific in mind? |
|
what do you think of my comment above, would you agree that this change makes the matches method more expensive? I think what you propose is anti-pattern for the IntersectsVisitor API. The point of having two methods is that computing relates is in general more expensive than compute matches and therefore matches should never call relates. |
Probably. I have tweaked the code to avoid the below check. Let me know if you still feel it becomes expensive.
IMHO, relates and matches can be same method given we don't resolve the relation further when it intersects. |
|
|
||
| private Relation relate(byte[] minPackedValue, byte[] maxPackedValue) { | ||
| private Relation relateHelper( | ||
| byte[] minPackedValue, byte[] maxPackedValue, boolean needCrossOrInside) { |
There was a problem hiding this comment.
I don't see the point of this indirection with boolean needCrossOrInside. It makes the code less readable and doesn't seem to remove a whole lot of redundant code.. I'd rather we just add a comment that explains the extra check we do in relate().
I'm not deeply familiar with this part of Lucene, but it seems like the checks here can have some non-obvious overheads? What would be a good benchmark to surface them? Is it possible to extend an existing benchmark?
There was a problem hiding this comment.
I am not completely convinced about the readability part, since redundancy requires changing code at multiple places. Maybe there is more readable version of this code, that I am unable to think of. But I do see your point about non-obvious overheads. Initially, I assumed that method inlining should be able to mitigate this overhead, but the default size limit for inlining is fairly small 35 bytes.
% java -XX:+PrintFlagsFinal -version | grep MaxInlineSize
intx C1MaxInlineSize = 35 {C1 product} {default}
intx MaxInlineSize = 35 {C2 product} {default}
openjdk version "21.0.4" 2024-07-16 LTS
jainankitk
force-pushed
the
lucene-13598
branch
from
d9f9398 to
97d89c6
Compare
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
|
In general, I really appreciate that you're looking for opportunities to cleanup the codebase and find ways to avoid duplicated logic. Thanks @jainankitk ! At the same time, I don't personally agree with making this change. I do like that you've found a way to avoid the unnecessary work (nice improvement from the original approach that fully delegated to |
This reverts commit caa665e.
Signed-off-by: Ankit Jain <[email protected]>
Signed-off-by: Ankit Jain <[email protected]>
Thanks @gsmiller for providing this feedback. It also seemed bit odd to be passing same packed value point twice, but still wanted to get opinion on it. After removing the clunky parts, very small change remaining that probably makes sense. Would have liked to not make the change at 2 places, but it seems there is no good way. Just for context, I came across this code path while reviewing the changes for ApproximateRangeQuery in Opensearch. I am wondering if we can make some other changes in |
|
@jainankitk thanks for the iterations! I'm fine with making this change as you currently having. I'll get it merged. Thanks! |
5 participants
Description
Delegating the matches in PointRangeQuery weight to relate method
Issue
Resolves #13598