EdgeQuery.initCovering: Use a single iterator

[benguild]

This also fixes #247, but that should probably be merged first while this is reviewed.

Documented previously as the TODO in the code in both Go and C++. This aims to resolve it. If this is not sufficient, please let me know and I'll take a look. I took a shot at the C++ as well (in a draft PR) to match: google/s2geometry#494

[jmr]

Is it faster? It looks more complicated.

[benguild]

Is it faster? It looks more complicated.

Honestly, probably not that much. It's just avoiding the iterator cloning.

[benguild]

@rsned If you have a moment, would appreciate eyes/consideration on this. Thanks in advance.

[jmr]

Is it faster? It looks more complicated.

Honestly, probably not that much.

There are benchmarks. Could you run them and report the diffs?

[benguild]

This seems marginal if not performance neutral from a benchmark perspective, the numbers were just noise like ~0.7% or around 1-2% different at least on my machine. Feel free to verify and judge, and decide based on code clarity and consistency with C++ or whatever might be best.

[benguild]

Just checking in on this, would be great to consider next steps.

[benguild]

Since #247 is already merged which fixed the bug, and the TODO resolution results in similarly performant (or slightly slower) code, it may be better to remove the TODO instead unless there's a higher level plan here that I'm not aware of. Note that we have draft mirrored on C++ google/s2geometry#494

[panmari]

This seems marginal if not performance neutral from a benchmark perspective, the numbers were just noise like ~0.7% or around 1-2% different at least on my machine. Feel free to verify and judge, and decide based on code clarity and consistency with C++ or whatever might be best.

For completness sake, could you still post the benchmark results? E.g. using https://pkg.go.dev/golang.org/x/tools/cmd/benchcmp, which helps greatly compiling an overview of multiple benchmark runs.

[benguild]

This seems marginal if not performance neutral from a benchmark perspective, the numbers were just noise like ~0.7% or around 1-2% different at least on my machine. Feel free to verify and judge, and decide based on code clarity and consistency with C++ or whatever might be best.

For completness sake, could you still post the benchmark results? E.g. using https://pkg.go.dev/golang.org/x/tools/cmd/benchcmp, which helps greatly compiling an overview of multiple benchmark runs.

I don't think I wrote one, it's just the one that was pre-existing. (I don't have this code checked out at the moment but if you do and can check out the branch you could run it)

[benguild]

At this point, I think it's perhaps best to close this with the suggestion the TODO be reconsidered. It's an interesting refactor but it's not clear if the benefits are there. The C++ sibling PR is a draft anyway, and for this to remain in sync it'd need attention on both sides depending on what's best.