cpython icon indicating copy to clipboard operation
cpython copied to clipboard

Fix issues gh-86199 gh-86795

Open MojoVampire opened this issue 3 years ago • 7 comments
trafficstars

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 avatar May 02 '22 20:05 MojoVampire

All commit authors signed the Contributor License Agreement.
CLA signed

cpython-cla-bot[bot] avatar May 02 '22 20:05 cpython-cla-bot[bot]

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 avatar May 02 '22 20:05 MojoVampire

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 avatar May 04 '22 03:05 MojoVampire

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.

erlend-aasland avatar May 08 '22 20:05 erlend-aasland

@MojoVampire?

markshannon avatar Jun 10 '22 15:06 markshannon

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

erlend-aasland avatar Sep 04 '22 22:09 erlend-aasland

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

MojoVampire avatar Nov 09 '22 22:11 MojoVampire