Skip to content

Fix issues gh-86199 gh-86795 #92192

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix issues gh-86199 gh-86795 #92192

wants to merge 6 commits into from
+105 −1

Conversation

MojoVampire
Copy link
Contributor

@MojoVampire MojoVampire commented May 2, 2022 •
edited
Loading

Fix issues #86199 and #86795 by centralizing copying of keyword arguments in PyObject_Call only when needed

gh-86199: `**kwargs` are not copied by the compiler when they are the only source of keyword arguments for a call. -

gh-86795: `PyObject_Call` makes copies of the keyword arguments dictionary when calling non-vectorcall functions to consistently ensure the caller's `dict` is not modified, whether the `dict` comes from the eval loop or a direct C extension call to `PyObject_Call`, matching the documented equivalence with `callable(*args, **kwargs)`. Reduces overhead of `callable(**kwargs)` ~30%.

@MojoVampire MojoVampire requested a review from markshannon as a code owner May 2, 2022 20:49
@MojoVampire
Copy link
Contributor Author

MojoVampire commented May 2, 2022 •
edited
Loading

Hmm... I've actually signed the CLA before under the old system, but it looks like the old private GitHub e-mail I was using doesn't work anymore (didn't realize this when I committed with it still configured in my .gitconfig). Not sure how to fix this retroactively. Not sure which of the several valid e-mails I now have should be the one attached to the contributor agreement for that matter.

Figured it out, managed to reset the authorship and repush so it's now credited to my new private GitHub e-mail that CLA-bot recognizes, so everything should be good now.

MojoVampire added 2 commits May 2, 2022 20:59
@MojoVampire
Fix python#86199 and python#86795 by:
52b6ec4 
Preventing compiler from inserting unnecessary dict build and merge when only **kwargs passed
 Make PyObject_Call copy any incoming keyword argument dicts when the callable in question is not vectorcall to avoid possibility of callee modifying caller's dict
Add tests that verify documented equivalence between callable(**kwargs) and PyObject_Call(callable, (), kwargs)
@MojoVampire
Fix python#86199 and python#86795 by:
b205d6b 
Preventing compiler from inserting unnecessary dict build and merge when only **kwargs passed
Make PyObject_Call copy any incoming keyword argument dicts when the callable in question is not vectorcall to avoid possibility of callee modifying caller's dict
Add tests that verify documented equivalence between callable(**kwargs) and PyObject_Call(callable, (), kwargs)
@MojoVampire MojoVampire force-pushed the fix-issues-86199-86795 branch from 4a8517f to b205d6b Compare May 2, 2022 21:01
@MojoVampire MojoVampire changed the title Fix issues 86199 86795 Fix issues #86199 #86795 May 2, 2022
@MojoVampire MojoVampire changed the title Fix issues #86199 #86795 Fix issues gh-86199 gh-86795 May 2, 2022
markshannon
markshannon reviewed May 2, 2022
View reviewed changes
erlend-aasland
erlend-aasland reviewed May 3, 2022
View reviewed changes
erlend-aasland
erlend-aasland reviewed May 3, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, apart from one more reST nit.

Also: @mentioning people in commit messages has historically led to an awful lot of unneeded pinging from forks, other PR's, etc. I don't know if GitHub addressed this problem1, but I find it best to stay on the cautious side, and just not do that; reducing the number of pings is a welcome consideration 😉

Footnotes

  1. UPDATE: confirming that I actually got an extra ping because of the commit message ↩

methane
methane reviewed May 4, 2022
View reviewed changes
@MojoVampire @erlend-aasland
Update Misc/NEWS.d/next/Core and Builtins/2022-05-02-20-19-06.gh-issu…
4b20dae 
…e-86199.IZbF0m.rst

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
@MojoVampire
Copy link
Contributor Author

Looks good, apart from one more reST nit.

Accepted as given.

Also: @mentioning people in commit messages has historically led to an awful lot of unneeded pinging from forks, other PR's, etc. I don't know if GitHub addressed this problem1, but I find it best to stay on the cautious side, and just not do that; reducing the number of pings is a welcome consideration 😉

Oops, did not know that would happen. Will avoid in the future.

@MojoVampire
Fast path for completely empty dict is faster than presizing when inp…
19db1da 
…ut dict is plain dict with no history of deletions (the common case)
methane
methane approved these changes May 4, 2022
View reviewed changes
Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
Contributor

Please resolve conflicts.

Also, please fix the PR title to gh-86795: <insert short summary of the actual change here>. You can describe the change in more detail (and mention gh-86199) in the PR body; this will make it easier for the core dev who merges this to create a sensible merge commit message.

@markshannon
Copy link
Member

@MojoVampire?

@erlend-aasland
Copy link
Contributor

@MojoVampire, are you planning to follow up this PR? If not, I suggest closing it.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Sep 4, 2022
@MojoVampire
Copy link
Contributor Author

Sorry, I've had a hell of a year. I'm going to try to rebase this soon.

@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Nov 11, 2022
@python python deleted a comment Apr 7, 2025
@python-cla-bot
Copy link

The following commit authors need to sign the Contributor License Agreement:

CLA signed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@methane methane methane approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@erlend-aasland erlend-aasland Awaiting requested review from erlend-aasland

Assignees
No one assigned
Labels
awaiting changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MojoVampire @erlend-aasland @markshannon @methane @bedevere-bot