Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Oct 15, 2024

@picnixz picnixz force-pushed the fix/memoryview-sequence-api-reversed-125420 branch from a95b097 to 4485dea Compare October 15, 2024 08:28
@JelleZijlstra
Copy link
Member

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.

@picnixz
Copy link
Member Author

picnixz commented Oct 15, 2024

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).

@JelleZijlstra
Copy link
Member

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

@picnixz
Copy link
Member Author

picnixz commented Oct 17, 2024

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)

@rhettinger rhettinger removed their request for review October 17, 2024 23:49
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long review--a lot of these are formatting changes, but there are a few things that certainly need to get fixed.

@picnixz picnixz requested a review from ZeroIntensity October 25, 2024 20:11
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a bit of stress testing locally, and despite my best efforts, I was unable to get this to break 😉. LGTM, thank you!

@picnixz picnixz added the stale Stale PR or inactive for long period of time. label Jan 23, 2025
@picnixz
Copy link
Member Author

picnixz commented Jan 23, 2025

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 picnixz closed this Jan 23, 2025
@picnixz picnixz deleted the fix/memoryview-sequence-api-reversed-125420 branch January 23, 2025 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants