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

[jimmylai]

https://bugs.python.org/issue33521

[1st1]

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

[1st1]

Move ); to the previous line, so that

intclass_has_attr=_PyObject_HasAttrId(
class, &PyId__asyncio_future_blocking);

[1st1]

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]

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]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

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

[asvetlov]

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

[1st1]

Good idea.

[jimmylai]

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

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]

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.

[vstinner]

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?

[1st1]

I suggested to use Py_TYPE as well.

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

[1st1]

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

[jimmylai]

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

Use 4-space indentation.

[jimmylai]

fixed.

[serhiy-storchaka]

Make the argument positional-only.

[vstinner]

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

[serhiy-storchaka]

Is it needed?

[jimmylai]

class may not have attr _asyncio_future_blocking
It's needed.

[serhiy-storchaka]

obj_attr is leaked here.

[jimmylai]

added Py_DECREF(obj_attr);

[jimmylai]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

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

[asvetlov]

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

[csabella]

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

[kumaraditya303]

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.