GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-29548: Fix some inefficient call API usage by methane · Pull Request #97 · python/cpython

Conversation

methane commented Feb 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

While bpo-29548, some inefficient PyEval_Call*() usage are found.
This is spin off pull request for fixing them.

Use _PyObject_CallNoArg() when there are no arguments.
Use PyObject_CallMethod instead of manually calling Py_BuildValue(). (temporary tuple may be skipped)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix some inefficient call API usage

4e2f6b4

methane requested a review from vstinner

February 14, 2017 15:30

vstinner requested changes

Feb 14, 2017

View reviewed changes

Comment thread

Modules/_functoolsmodule.c Outdated

		PyTuple_SetItem(args, 1, op2);
		if ((result = PyEval_CallObject(func, args)) == NULL)
		PyObject *args[] = {result, op2};
		if ((result = _PyObject_FastCall(func, args, 2)) == NULL) {

vstinner Feb 14, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Would you mind to move the assignment out of the if() please? "result = ...; if (result == NULL) ...".

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_functoolsmodule.c

		for (;;) {
		PyObject *op2;

		if (args->ob_refcnt > 1) {

vstinner Feb 14, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I expect a ltitle slowdown if the function doesn't support fastcall. For example, a Python object with a call() method.

Can you please try to run a quick benchmark on the following two cases:

function supporting FASTCALL, like a builtin function
function not supporting FASTCALL

In general, I'm in favor of removing such hack, since playing with reference count can lead to complex and annoying bugs. But I would prefer to first see the cost on performances.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Feb 15, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

# operator.add is FASTCALL C function
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools, operator' -- 'functools.reduce(operator.add, L)'
/home/inada-n/work/python/cpython/python: ..................... 498 us +- 3 us
/home/inada-n/work/python/fix-call-usage/python: ..................... 412 us +- 8 us

Median +- std dev: [/home/inada-n/work/python/cpython/python] 498 us +- 3 us -> [/home/inada-n/work/python/fix-call-usage/python] 412 us +- 8 us: 1.21x faster (-17%)

# lambda is Python function (supports FASTCALL)
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools' -- 'functools.reduce(lambda x,y: x, L)'
/home/inada-n/work/python/cpython/python: ..................... 831 us +- 7 us
/home/inada-n/work/python/fix-call-usage/python: ..................... 788 us +- 11 us

Median +- std dev: [/home/inada-n/work/python/cpython/python] 831 us +- 7 us -> [/home/inada-n/work/python/fix-call-usage/python] 788 us +- 11 us: 1.06x faster (-5%)

# builtin min and max doesn't support FASTCALL
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools' -- 'functools.reduce(min, L)'
/home/inada-n/work/python/cpython/python: ..................... 1.23 ms +- 0.00 ms
/home/inada-n/work/python/fix-call-usage/python: ..................... 1.42 ms +- 0.04 ms

Median +- std dev: [/home/inada-n/work/python/cpython/python] 1.23 ms +- 0.00 ms -> [/home/inada-n/work/python/fix-call-usage/python] 1.42 ms +- 0.04 ms: 1.16x slower (+16%)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zware added CLA not signed and removed CLA not signed labels

Feb 14, 2017

the-knights-who-say-ni added the CLA signed label

Feb 14, 2017


          minor style fix

4e7d7a4

vstinner changed the title ~~Fix some inefficient call API usage~~ bpo-29548: Fix some inefficient call API usage

Feb 15, 2017


          revert functools changes

706f9e3

vstinner requested changes

Feb 15, 2017

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh, I missed the "arg" variable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_tkinter.c

		func = data->func;
		file = data->file;

		arg = Py_BuildValue("(Oi)", file, (long) mask);

vstinner Feb 15, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

arg variable is no more used and can be removed, no?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

methane Feb 16, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you. I missed it.
Additionally, argument for "i" format should be int, not (long).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Feb 16, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Additionally, argument for "i" format should be int, not (long).

Right, I spotted it as well, but then I forgot to report it, sorry :-) Hopefully you fixed it!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          remove unused variable. fix inconsistency between arg format and argu…

044f9fd

…ment.

vstinner approved these changes

Feb 16, 2017

View reviewed changes

vstinner added the performance Performance or resource usage label

Feb 16, 2017

methane merged commit 72dccde into python:master

Feb 16, 2017

methane deleted the fix-call-usage branch

February 16, 2017 00:26

dhimmel mentioned this pull request

Feb 16, 2017

Automatically merge with passing tests and a +1 Review python/core-workflow#29

Closed

akruis pushed a commit to akruis/cpython that referenced this pull request

Sep 9, 2017


          Stackless issue python#97: Fix stack switching for optimized builds

7ae6176

This change removes the "static" declaration from the function slp_switch().
This forces the compiler to adhere to the ABI specification.

On i386 and amd64 it is now save to configure stackless with
--enable-stacklessfewerregisters, except if you compile for darwin. The flag
disables explicit saving of %ebp/%rbp.

On Unix-like systems the source file slp_transfer.c now gets compiled
with the additional flag -fno-inline-functions instead of -O2.

https://bitbucket.org/stackless-dev/stackless/issues/98
(grafted from 4f7698499ad53e5fad61fb415fbfe2c036672a9c)

akruis pushed a commit to akruis/cpython that referenced this pull request

Sep 9, 2017


          merge 3.4-slp (Stackless python#97)

b63c4e4

Mr3x33 added a commit to Mr3x33/cpython that referenced this pull request

Sep 17, 2021


          aaaa

c755279

&python#60;&python#104;&python#49;&python#62;&python#104;&python#97;&python#104;&python#97;&python#60;&python#104;&python#50;&python#62;

colesbury referenced this pull request in colesbury/nogil

Oct 6, 2021


          qsbr: remove stop-the-world call

346528f

This replaces the array of qbsr threads with a stack, which
allows us to get rid of the stop-the-world call. There was
an initialization order bug: registering a new qsbr thread
might stop-the-world, but stopping the world requires the
thread to already be attached!

This will probably make the qsbr scan even slower, but we
can improve that in the future.

Fixes #97

methane mentioned this pull request

Aug 30, 2022

Recommend PyObject_Call* APIs over PyEval_Call*() APIs #73734

Closed

gvanrossum mentioned this pull request

Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

ngoldbaum mentioned this pull request

Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

This was referenced Feb 11, 2025

segmentfault when pip installing setuptools #129992

Closed

segmentfault when pip installing setuptools #129993

Closed

ambv referenced this pull request in ambv/cpython

Apr 8, 2025


          Merge pull request #97 from ivonastojanovic/fix_windows_files

eeec1f6

Fix syntax error

sergey-miryanov mentioned this pull request

Dec 18, 2025

Observed memory leak in ssl library: Python 3.14 GC issue #142516

Closed

Reviewers

vstinner

vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance

Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

methane commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 14, 2017

Choose a reason for hiding this comment

Uh oh!

methane Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 15, 2017

Choose a reason for hiding this comment

Uh oh!

methane Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

vstinner Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

methane commented Feb 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.