Skip to content
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

perf: Optimize iterator advance() call #4237

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Oct 12, 2022

Description

Inspired by #4232, I found one other place where we ignore the return value of C-API function and blindly call PyErr_Occurred(). Worse, it's in a very hot piece of code, (the advance() function for iterators in C++ bindings). The official C-API documentation example code clearly shows that PyErr_Occurred() check is only necessary when the iteration terminates (ie. the next pointer returned is not null). PyErrOccurred() involves a good deal of pointer chasing so best to avoid having to so every step: https://github.com/vstinner/pythondev/blob/d7a8d069e5a5d7ad26503f98a2d29c1508a03e73/assembly.rst

Suggested changelog entry:

* perf: optimize iterator advancement in C++ bindings.

@Skylion007 Skylion007 changed the title chore: Optimize iterator advance() call perf: Optimize iterator advance() call Oct 12, 2022
@Skylion007 Skylion007 requested a review from rwgk October 12, 2022 17:51
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

The Python documentation for this is indeed super clear.

@Skylion007 Skylion007 merged commit 8781daf into pybind:master Oct 12, 2022
@Skylion007 Skylion007 deleted the py-iter-check-nullptr branch October 12, 2022 20:46
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 12, 2022
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants