Performance regression on LatLonPoint#newPolygonQuery

[iverase]

Description

I just notice a big performance regression on polygon queries using LatLonPoint field in lucene geo benchmarks:

I checked and the regression was introduced by this change: #1017.

My suspicion is that before this change, SpatialQuery was calling the method #getSpatialVisitor() once for the whole index but in the new version is calling it once per segment. This method might be expensive for LatLonPoint queries, threfore the regression.

@nknize FYI

Version and environment details

No response

[iverase]

close in #11825

[nknize]

Just seeing this. That's exactly what it would be! Snuck in one of those last commits on the long running PR. Thanks for refactoring and merging @iverase!

[iverase]

Fix seems to bring performance back to previous levels:

[mikemccand]

Thanks for catching this @iverase and the quick fix, and the follow-on issue to better detect such regressions before release: #11827

[nknize]

What's annoying is how incredibly trappy this override logic is. That a method call literally moving from createWeight to getScorerSupplier results in a 72.2% regression even slipped by me before merge doesn't bode well for new committers. And then it sat in regression until an entire company was interested in releasing.

I wonder if we can do better? Like maybe figure out better guardrails in these methods? Perhaps by something as simple as a rename (e.g., getScorerSupplierPerSegment) to signal one happens per segment? This isn't the first and will certainly not be the last time an expensive operation accidentally slips to a critical path. Any other ideas how to lower the bar here for new committers?