Early terminate visit BKD leaf when current value greater than upper point in sorted dim.

[vsop-479]

Since values are sorted on the sorted dim in BKD leaf, early terminate the visit when current packed value greater than upper point in sorted dimension, may get a better performance.

Here are some performance data:

1 dim point

Visit method for low cardinality points:

baseline ns	candidate ns	speedup
52017	41400	20.4%

Whole search for low cardinality points:

baseline ns	candidate ns	speedup
242058	228225	5.7%

Whole search for high cardinality points:

baseline ns	candidate ns	speedup
984615	868358	11.8%

2 dim point

Whole search for low cardinality points:

baseline ns	candidate ns	speedup
1748439	1656944	5.2%

Whole search for high cardinality points:

baseline ns	candidate ns	speedup
1655624	1446875	12.6%

[vsop-479]

@jpountz Please take a look when you get a chance!

[vsop-479]

@iverase I replaced int values with static variables. Please take a look.
Actually, i used enum to define the match states in pre version. but it downgraded the performance a little.
Static variables is good, but do you think it is ok to use enum to make code graceful, even through there is little performance downgrade?

[iverase]

My main concern here is that the concept of SORTED dimension does not exist in the PointValues API. If you have a look to the javadocs when visiting a leaf node:

    /**
     * Called for all documents in a leaf cell that crosses the query. The consumer should
     * scrutinize the packedValue to decide whether to accept it. In the 1D case, values are visited
     * in increasing order, and in the case of ties, in increasing docID order.
     */

It only constraints the 1D case but in higher dimensions there is no constraint how data is visited. The concept of SORTED dimension sounds to me an implementation detail that should not be leaked to the public API.

[iverase]

I have concerns with this change as I have the impression that we are leaking implementation details into the public API which is not a good idea in general. Maybe other folks have a different opinion but I don't think the leaves sorted dimensions should be part of the public API, or at least we need to make it explicit before this change.

[vsop-479]

@iverase Does it make sense to you if MatchState defined in other class, such as BKDReader or IntersectVisitor, and only leave the sorted dimension in IntersectVisitor' visit method as a parameter?
I just implemented visitWithState in PointRangeQuery, and there is some speed up. I think other queries, such as LatLonPointDistanceQuery, which use a similar IntersectVisitor, can get a better performance too.

[iverase]

My recommendation is to use the following method as you are just trying to flag if the visit method needs to keep processing points.

  /** Similar to {@link IntersectVisitor#visit(int, byte[])} but data is visited in 
   * increasing order on the {@sortedDim}, and in the case of ties, in increasing docID order. 
   * Implementers can stop processing points on the leaf by returning false when for example the 
   * sorted dimension value is too high to be matched by the query. 
   * 
   * @return true if the visitor should continue visiting points on this leaf, otherwise false. 
   * */

 default boolean visitWithSortedDim(int docID, byte[] packedValue, int sortedDim) throws IOException { 
       visit(docID, packedValue); 
        return true; 
   }

I would remove the "inverse" case as the inverse visitors are implementation details and IMHO should not be part of the API.

just implemented visitWithState in PointRangeQuery, and there is some speed up

We should run the performance test in the [lucene benchmarks](lucene benchmarks) to check if there are slowdowns due to the extra check for each visited point.

[vsop-479]

@iverase Thanks for your recommendation, it may makes code more clear.
I will try to implement it, and run the performance test.

[vsop-479]

@iverase Here is a performance test from luceneutil.

box-points	baseline	candidate	Diff
BEST M hits/sec	101.09	103.64	2.5%
BEST QPS	102.87	105.46	2.5%

I only implemented visitWithSortedDim in PointRangeQuery (without "inverse" case) temporarily, so I just ran the box's points test.

run the performance test in the [lucene benchmarks]

Do you mean the off the shelf benchmark in lucene self?

[iverase]

Do you mean the off the shelf benchmark in lucene self?

One case I am interested in is the geo benchmarks. It is not clear to me if some of those queries (e.g polygon query) can effectively take advantage of this change so it would be good to know that they are not slowed down.

[vsop-479]

@iverase
Here is a performance data of geo cases. There are some slowdown due to the extra check.

query	metric	baseline	candidate	Diff
poly-10-geo3d	BEST M hits/sec	68.13	67.99	-0.2%
--	BEST QPS	43.07	42.99	-0.1%
box-geo3d	BEST M hits/sec	73.63	72.86	-0.1%
--	BEST QPS	74.92	74.14	-0.1%

I want to add a enhanced IntersectVisitor type or add a flag to IntersectVisitor, for queries which have already implemented visitWithSortedDim. And leave queries which haven't or couldn't implement visitWithSortedDim, use the original visit method to avoid the unnecessary checks.
Does it make sense to you?

[vsop-479]

@iverase I implemented visitWithSortedDim in LatLonPointDistanceQuery. Please take a look when you get a chance!

BTW, The contents of getIntersectVisitor and getInverseIntersectVisitor in LatLonPointDistanceQuery and PointRangeQuery are identical. Should we move them to a DefaultIntersectVisitor and make match/relate abstract in it. I mean in a new PR without this change.

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

[vsop-479]

@iverase
I implemented visitWithSortedDim for pointCount in PointRangeQuery. Please take a look when you get a chance.

Or Do you have any other ideas to terminate visiting points, which already greater than upperPoint? Even just for 1D points.

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

[vsop-479]

if there are slowdowns due to the extra check for each visited point.

Maybe this extra check can be weakened by Branch predictor

[github-actions]

This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the [email protected] list. Thank you for your contribution!

[vsop-479]

I would remove the "inverse" case as the inverse visitors are implementation details and IMHO should not be part of the API.

FWIW, I implemented "inverse" case with adding a VisitState.

[vsop-479]

I will try to measure the performance.