Implement set iterators in terms of limited API

[alex]

No description provided.

[davidhewitt]

As this looks like it could have performance drawbacks, can we perhaps consider in this case having two implementations depending on the API available?

Or at least a benchmark showing that the difference is not meaningful.

(We have some benchmarks for iterating list and dict so should be able to duplicate them easily for set.)

[alex]

I'll do a PR adding the benchmark to master, then we compare performance between teh two.

[alex]

Are there instructions on running benchmarks?

[davidhewitt]

cargo +nightly bench should do it - though great point, should add this to contributing.md

[alex]

The measurements are a bit noisy, because I'm running on a laptop, but bench_set from #1168 is 10-15% slower. Is that so much of a slowdown that it needs 2x the impls? My instinct is that it's not worth it unless someone has a specific use case.

[kngwyu]

How about making the current set iterator #[cfg(not(Py_LIMITED_API))]? (EDITED: I noticed that it is already not(Py_LIMTED_API)
We can do the same thing by PyIterator::from_object as the new implementation and isn't it insufficient?

[davidhewitt]

+1 - I think that as we've already got both implementations written, and a single test will cover both APIs depending on the features, it's easy for us to support both.

10-15% may not be huge, but for some people the choice of Rust is for everything to be as fast as possible, so they will still appreciate this.

[davidhewitt]

Thanks, LGTM!

[kngwyu]

I meant 'probably we don't need PySetIterator for #[cfg(Py_LIMITED_API)]'.

[alex]

@kngwyu we do -- it needs to PyIterator is an iterator of PyResult, not an iterator of PyAny. You need PySetIterator to unwrap the PyResult (which is fine because in reality PySet's iterator can never error).

[kngwyu]

OK, that makes sense.