Skip to content

bpo-33521: Add 1.32x faster C implementation of asyncio.isfuture(). #6876

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

Closed
jimmylai wants to merge 2 commits into from
Closed

bpo-33521: Add 1.32x faster C implementation of asyncio.isfuture(). #6876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9aa3417
bpo-33521 Optimize asyncio.isfuture by providing C implementation.
May 15, 2018
d689d5f
bpo-33521: Add 1.32x faster C implementation of asyncio.isfuture().
May 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Lib/asyncio/futures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,4 +381,5 @@ def wrap_future(future, *, loop=None):
pass
else:
# _CFuture is needed for tests.
isfuture = _asyncio.isfuture
Future = _CFuture = _asyncio.Future
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2018-05-15-16-00-08.bpo-33521.GjWHfn.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add 1.32x faster C implementation of asyncio.isfuture().
46 changes: 46 additions & 0 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,51 @@ is_coroutine(PyObject *coro)
return has_it;
}

/*[clinic input]
_asyncio.isfuture

obj: object
Copy link
Member

@serhiy-storchaka serhiy-storchaka May 20, 2018

Choose a reason for hiding this comment

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

Make the argument positional-only.

Copy link
Member

@vstinner vstinner May 22, 2018

Choose a reason for hiding this comment

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

In pratice, it means adding "/" on a new line, aligned with "obj", and run "make clinic" again.

/

Return True if obj is a Future instance.

This returns True when obj is a Future instance or is advertising
itself as duck-type compatible by setting _asyncio_future_blocking.
See comment in Future for more details.

[clinic start generated code]*/

static PyObject *
_asyncio_isfuture(PyObject *module, PyObject *obj)
/*[clinic end generated code: output=3c79d083f507d4fa input=a71fdef4d9b354b4]*/
{
_Py_IDENTIFIER(__class__);
Copy link
Contributor

@asvetlov asvetlov May 19, 2018

Choose a reason for hiding this comment

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

Why not use Future_CheckExact() / Future_Check() as very fast happy path here?

Copy link
Member

@1st1 1st1 May 19, 2018

Choose a reason for hiding this comment

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

Good idea.

Copy link
Author

@jimmylai jimmylai Jun 18, 2018

Choose a reason for hiding this comment

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

@asvetlov @1st1 isfuture check if _asyncio_future_blocking exists and is not None because mock object has _asyncio_future_blocking as None and we want to return isfuture() as False in this case.
See test case

cpython/Lib/test/test_asyncio/test_futures.py

Lines 123 to 124 in 8425de4

# As `isinstance(Mock(), Future)` returns `False`
self.assertFalse(asyncio.isfuture(mock.Mock()))

Do we really want to use Future_CheckExact to return fast?

PyObject* class = _PyObject_GetAttrId(obj, &PyId___class__);
Copy link
Member

@serhiy-storchaka serhiy-storchaka May 20, 2018

Choose a reason for hiding this comment

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

It is better to use Py_TYPE() for convenience and speed . There is a subtle difference between the type and the __class__ attribute, but it is usually ignored when write C accelerations.

Copy link
Member

@vstinner vstinner May 22, 2018

Choose a reason for hiding this comment

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

I suggested to @jimmylai to get the class attribute to make sure that the C implementation behaves as the Python implementation: see PEP 399. If someone wants to use type(), I would prefer to see the same change in the Python implementation as well.

@1st1, @asvetlov: Do you know the rationale for checking class here?

Copy link
Member

@1st1 1st1 May 22, 2018

Choose a reason for hiding this comment

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

I suggested to use Py_TYPE as well.

I think it was I who used __class__ there, and there's no rationale behind that :)

Copy link
Member

@1st1 1st1 May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So the Python version can be updated to use type() too.

Copy link
Author

@jimmylai jimmylai Jun 18, 2018

Choose a reason for hiding this comment

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

@1st1 @serhiy-storchaka
use type() won't work.
Original isfuture: return (hasattr(obj.__class__, '_asyncio_future_blocking') and obj._asyncio_future_blocking is not None)

Use type: return (hasattr(type(obj), '_asyncio_future_blocking') and obj._asyncio_future_blocking is not None)

Unit tests fail when use type()
Did I use type() wrong?

======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.CFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
    self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true

======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.CSubFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
    self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true

======================================================================
FAIL: test_isfuture (test.test_asyncio.test_futures.PyFutureTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jimmylai/workspace/cpython/Lib/test/test_asyncio/test_futures.py", line 131, in test_isfuture
    self.assertTrue(asyncio.isfuture(mock.Mock(type(f))))
AssertionError: False is not true

if (class == NULL) {
return NULL;
}
_Py_IDENTIFIER(_asyncio_future_blocking);
int class_has_attr = _PyObject_HasAttrId(
Copy link
Member

@serhiy-storchaka serhiy-storchaka May 20, 2018

Choose a reason for hiding this comment

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

Is it needed?

Copy link
Author

@jimmylai jimmylai Jun 18, 2018

Choose a reason for hiding this comment

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

class may not have attr _asyncio_future_blocking
It's needed.

class, &PyId__asyncio_future_blocking);
Py_DECREF(class);
if (class_has_attr < 0) {
return NULL;
}
if (!class_has_attr) {
Py_RETURN_FALSE;
}
PyObject* obj_attr = _PyObject_GetAttrId(obj, &PyId__asyncio_future_blocking);
if (obj_attr == NULL) {
return NULL;
}
if (obj_attr != Py_None) {
Py_DECREF(obj_attr);
Py_RETURN_TRUE;
}
Py_DECREF(obj_attr);
Py_RETURN_FALSE;
Copy link
Member

@serhiy-storchaka serhiy-storchaka May 20, 2018

Choose a reason for hiding this comment

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

obj_attr is leaked here.

Copy link
Author

@jimmylai jimmylai Jun 18, 2018

Choose a reason for hiding this comment

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

added Py_DECREF(obj_attr);

}


static PyObject *
get_future_loop(PyObject *fut)
Expand Down Expand Up @@ -3276,6 +3321,7 @@ PyDoc_STRVAR(module_doc, "Accelerator module for asyncio");

static PyMethodDef asyncio_methods[] = {
_ASYNCIO_GET_EVENT_LOOP_METHODDEF
_ASYNCIO_ISFUTURE_METHODDEF
_ASYNCIO_GET_RUNNING_LOOP_METHODDEF
_ASYNCIO__GET_RUNNING_LOOP_METHODDEF
_ASYNCIO__SET_RUNNING_LOOP_METHODDEF
Expand Down
15 changes: 14 additions & 1 deletion Modules/clinic/_asynciomodule.c.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@
preserve
[clinic start generated code]*/

PyDoc_STRVAR(_asyncio_isfuture__doc__,
"isfuture($module, obj, /)\n"
"--\n"
"\n"
"Return True if obj is a Future instance.\n"
"\n"
"This returns True when obj is a Future instance or is advertising\n"
"itself as duck-type compatible by setting _asyncio_future_blocking.\n"
"See comment in Future for more details.");

#define _ASYNCIO_ISFUTURE_METHODDEF \
{"isfuture", (PyCFunction)_asyncio_isfuture, METH_O, _asyncio_isfuture__doc__},

PyDoc_STRVAR(_asyncio_Future___init____doc__,
"Future(*, loop=None)\n"
"--\n"
Expand Down Expand Up @@ -711,4 +724,4 @@ _asyncio__leave_task(PyObject *module, PyObject *const *args, Py_ssize_t nargs,
exit:
return return_value;
}
/*[clinic end generated code: output=b6148b0134e7a819 input=a9049054013a1b77]*/
/*[clinic end generated code: output=9ddba804d97cdf0d input=a9049054013a1b77]*/