optimize sequence conversion for list and tuple

[davidhewitt]

closes #2943

Avoid using PyObject_IsInstance for checking if lists or tuples are sequences, as we know they are always sequences.

[davidhewitt]

@ritchie46 would you be willing to run your benchmark suite built against this branch to verify this resolves the issue?

If so, we can prepare 0.18.2 patch release with this in a week or two.

[ritchie46]

@ritchie46 would you be willing to run your benchmark suite built against this branch to verify this resolves the issue?

If so, we can prepare 0.18.2 patch release with this in a week or two.

Yeap, seems much better; 👍

• 0.18: best of 5: 359 nsec per loop
• this_branch: best of 5: 193 nsec per loop

[davidhewitt]

Great - so I agree with you @adamreichold that it won't solve the general case (and all the discussion in #2943 about not needing PySequence at all for this particular case in polars).

I still think this optimization seems worth it for a common case?

[adamreichold]

I still think this optimization seems worth it for a common case?

Agreed. (I have not yet messaged bors to give the others a change to weigh in.)

[mejrs]

Do we have a benchmark for this? We should catch similar regressions in the future.

[adamreichold]

Do we have a benchmark for this? We should catch similar regressions in the future.

I agree that a benchmark would be helpful but I don't think this is an unexpected regression. I think everybody involved in the review expected this to decrease performance with the memoization of the collection.abc.Sequence type object being a partial mitigation due to that. From my perspective, the original report is rather about very specific use cases which are unable or unwilling to tolerate even the intentional change in performance.

[davidhewitt]

👍 I'll push a benchmark within a day or two.

[davidhewitt]

Benchmark pushed. On my machine these drop from the 100-200ns range to about 2ns 😄

[ritchie46]

Benchmark pushed. On my machine these drop from the 100-200ns range to about 2ns smile

Wow... That that's 1-3 cpu cycles 🙈

[davidhewitt]

Yes, checking for lists and tuples (and subclasses) can be done by checking for a bit set on the type object.

I should probably push a similar PR for dicts & mapping.

[davidhewitt]

Think we've given others a few days, I'd like to prep the next patch release soon.

bors r=adamreichold

[bors]

Build succeeded:

• conclusion

[jakob-keller]

Sounds fantastic, can't wait to give it a spin! Thanks to everyone involved!