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

jimmylai · 2018-05-15T20:02:52Z

https://bugs.python.org/issue33521

1st1 · 2018-05-17T16:25:47Z

Modules/_asynciomodule.c

+  _Py_IDENTIFIER(__class__);
+  PyObject* class = _PyObject_GetAttrId(obj, &PyId___class__);
+  if (class == NULL) {
+    return NULL;


Please use 4 spaces to ident your code (as the rest of _asynciomodule.c file).

1st1 · 2018-05-17T16:26:26Z

Modules/_asynciomodule.c

+  _Py_IDENTIFIER(_asyncio_future_blocking);
+  int class_has_attr = _PyObject_HasAttrId(
+      class, &PyId__asyncio_future_blocking
+  );


Move ); to the previous line, so that

int class_has_attr = _PyObject_HasAttrId( class, &PyId__asyncio_future_blocking);

1st1 · 2018-05-17T16:28:06Z

Modules/_asynciomodule.c

+    return NULL;
+  }
+  Py_DECREF(obj_attr);
+  if (obj_attr != Py_None) {


While this code is OK, you shouldn't use refs to Python objects after you DECREF them, it's a bad style. Please compare first, and decref after,

bedevere-bot · 2018-05-17T16:28:18Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

jimmylai · 2018-05-19T16:27:06Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-19T16:27:08Z

Thanks for making the requested changes!

@1st1: please review the changes made to this pull request.

asvetlov · 2018-05-19T20:25:30Z

Modules/_asynciomodule.c

+_asyncio_isfuture_impl(PyObject *module, PyObject *obj)
+/*[clinic end generated code: output=7ceb4ecaca613a86 input=b417c209efff25ec]*/
+{
+    _Py_IDENTIFIER(__class__);


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

@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?

bedevere-bot · 2018-05-19T20:25:37Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

serhiy-storchaka

I don't know whether it is worth to add C implementation of this function, but I think it can be simplified.

See also that the _asyncio_future_blocking attribute is used in the existing C code in suboptimal way.

serhiy-storchaka · 2018-05-20T05:59:46Z

Modules/_asynciomodule.c

+/*[clinic end generated code: output=7ceb4ecaca613a86 input=b417c209efff25ec]*/
+{
+    _Py_IDENTIFIER(__class__);
+    PyObject* class = _PyObject_GetAttrId(obj, &PyId___class__);


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.

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?

I suggested to use Py_TYPE as well.

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

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

@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

serhiy-storchaka · 2018-05-20T06:01:22Z

Modules/_asynciomodule.c

+    }
+    PyObject* obj_attr = _PyObject_GetAttrId(obj, &PyId__asyncio_future_blocking);
+    if (obj_attr == NULL) {
+      return NULL;


Use 4-space indentation.

serhiy-storchaka · 2018-05-20T06:06:08Z

Modules/_asynciomodule.c

+/*[clinic input]
+_asyncio.isfuture
+
+    obj: object


Make the argument positional-only.

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

serhiy-storchaka · 2018-05-20T06:08:16Z

Modules/_asynciomodule.c

+      return NULL;
+    }
+    _Py_IDENTIFIER(_asyncio_future_blocking);
+    int class_has_attr = _PyObject_HasAttrId(


Is it needed?

class may not have attr _asyncio_future_blocking
It's needed.

serhiy-storchaka · 2018-05-20T06:08:51Z

Modules/_asynciomodule.c

+        Py_DECREF(obj_attr);
+        Py_RETURN_TRUE;
+    }
+    Py_RETURN_FALSE;


obj_attr is leaked here.

added Py_DECREF(obj_attr);

jimmylai · 2018-06-18T14:05:24Z

I have made the requested changes; please review again.

bedevere-bot · 2018-06-18T14:05:27Z

Thanks for making the requested changes!

@asvetlov, @1st1: please review the changes made to this pull request.

asvetlov · 2018-08-02T20:13:13Z

@jimmylai the PR is failing on tests passing.
Could you take a look?

csabella · 2020-01-25T20:13:51Z

@jimmylai, please take a look at the last comment and please also resolve the merge conflict. Thank you!

kumaraditya303 · 2022-04-19T12:16:55Z

This PR is awaiting changes for over two years, tests were failing and has merge conflicts so I am closing it. If you are still interested you can create a new PR with the requested changes or this one can be reopened if needed.

bpo-33521 Optimize asyncio.isfuture by providing C implementation.

9aa3417

jimmylai requested review from 1st1 and asvetlov as code owners May 15, 2018 20:02

the-knights-who-say-ni added the CLA signed label May 15, 2018

bedevere-bot added the awaiting review label May 15, 2018

jimmylai changed the title ~~Add 1.32x faster C implementation of asyncio.isfuture().~~ bpo-33521: Add 1.32x faster C implementation of asyncio.isfuture(). May 15, 2018

vstinner self-requested a review May 17, 2018 06:57

1st1 requested changes May 17, 2018

View reviewed changes

bedevere-bot removed the awaiting review label May 17, 2018

bedevere-bot added the awaiting changes label May 17, 2018

jimmylai force-pushed the optimize_isfuture branch 2 times, most recently from c72d089 to 58ad925 Compare May 19, 2018 16:25

bedevere-bot removed the awaiting changes label May 19, 2018

bedevere-bot added the awaiting change review label May 19, 2018

asvetlov requested changes May 19, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 19, 2018

serhiy-storchaka reviewed May 20, 2018

View reviewed changes

jimmylai force-pushed the optimize_isfuture branch 3 times, most recently from 55c77d7 to 66e4586 Compare June 18, 2018 13:59

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

d689d5f

jimmylai force-pushed the optimize_isfuture branch from 66e4586 to d689d5f Compare June 18, 2018 13:59

bedevere-bot added awaiting change review and removed awaiting changes labels Jun 18, 2018

csabella added awaiting changes and removed awaiting change review labels Jan 25, 2020

AlexWaygood added performance Performance or resource usage topic-asyncio labels Apr 11, 2022

kumaraditya303 closed this Apr 19, 2022

jimmylai mannequin mentioned this pull request Apr 10, 2022

Add 1.32x faster C implementation of asyncio.isfuture(). #77702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jimmylai commented May 15, 2018 •

edited by bedevere-bot

Loading

1st1 May 17, 2018

1st1 May 17, 2018

1st1 May 17, 2018

bedevere-bot commented May 17, 2018

jimmylai commented May 19, 2018

bedevere-bot commented May 19, 2018

asvetlov May 19, 2018

1st1 May 19, 2018

jimmylai Jun 18, 2018

bedevere-bot commented May 19, 2018

serhiy-storchaka left a comment

serhiy-storchaka May 20, 2018

vstinner May 22, 2018

1st1 May 22, 2018

1st1 May 22, 2018 •

edited

Loading

jimmylai Jun 18, 2018

serhiy-storchaka May 20, 2018

jimmylai Jun 18, 2018

serhiy-storchaka May 20, 2018

vstinner May 22, 2018

serhiy-storchaka May 20, 2018

jimmylai Jun 18, 2018

serhiy-storchaka May 20, 2018

jimmylai Jun 18, 2018

jimmylai commented Jun 18, 2018

bedevere-bot commented Jun 18, 2018

asvetlov commented Aug 2, 2018

csabella commented Jan 25, 2020

kumaraditya303 commented Apr 19, 2022

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

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

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

Conversation

jimmylai commented May 15, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 17, 2018

jimmylai commented May 19, 2018

bedevere-bot commented May 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 19, 2018

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmylai commented Jun 18, 2018

bedevere-bot commented Jun 18, 2018

asvetlov commented Aug 2, 2018

csabella commented Jan 25, 2020

kumaraditya303 commented Apr 19, 2022

jimmylai commented May 15, 2018 •

edited by bedevere-bot

Loading

1st1 May 22, 2018 •

edited

Loading