cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-125420: implement `Sequence.__reversed__` API on `memoryview` objects

Open picnixz opened this issue 1 year ago • 4 comments

  • Issue: gh-125420

picnixz avatar Oct 15 '24 08:10 picnixz

Since this adds significant complexity and reversed() already works, I think we should only add this if it's actually faster than the current reversed(). If not, we can keep the current state at runtime and lie in the stub that __reversed__ exists; I feel that's relatively OK since it's a private dunder and people shouldn't be calling it directly anyway.

JelleZijlstra avatar Oct 15 '24 14:10 JelleZijlstra

Since this adds significant complexity and reversed() already works, I think we should only add this if it's actually faster than the current reversed(). If not, we can keep the current state at runtime and lie in the stub that reversed exists; I feel that's relatively OK since it's a private dunder and people shouldn't be calling it directly anyway.

I won't have the bandwidth to report the timings but I'll do it tomorrow. I won't be reachable until next Tuesday, so feel free to ignore this PR for now (we can always consider it later).

picnixz avatar Oct 15 '24 14:10 picnixz

There's some time left until the 3.14 feature freeze :)

JelleZijlstra avatar Oct 15 '24 14:10 JelleZijlstra

Some update: materializing the reversed iterator is now slower since precomputing the format is needed and other stuff is happening behind the scenes but consuming the iterator is faster once the number of items exceeds 32 (release non PGO). For smaller views, it's more or less the same speed (the call reversed(mv) is likely always slower with this specific implementation since we don't use a generic reversed object, but iterating will require a bit more work due to the internal format adjustment).

~I haven't implemented a vectorcall function so it's maybe why it's slower to materialize. (I'm just passing by)~

picnixz avatar Oct 17 '24 12:10 picnixz

Since we discussed about adding this one if needs arise, I will just close them and delete my branch for now (I have too many local branches and I don't want to keep stale PRs). We'll be able to reuse it later if needed.

picnixz avatar Jan 23 '25 14:01 picnixz