Skip to content
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.

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

bpo-32348: Optimize asyncio.Future schedule/add/remove callback. #4907

Merged
merged 3 commits into from
Dec 18, 2017

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Dec 16, 2017

@asvetlov
Copy link
Contributor

After the change C future has callback0 member but Python version has not.
Maybe adding callback0 to py future for sake of diversity reducing makes sense?

@1st1
Copy link
Member Author

1st1 commented Dec 17, 2017

I actually want to keep Python implementation simple (for educational purposes) and different (to catch any difference in semantics).

@asvetlov
Copy link
Contributor

Got it. Ok.

}
else {
assert(fut->fut_callback0 != NULL);
assert(fut->fut_callbacks == NULL);
Copy link
Member

Choose a reason for hiding this comment

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

These asserts seems too redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

dropped them

#define FI_FREELIST_MAXLEN 100
static futureiterobject *fi_freelist_head = NULL;
static futureiterobject *fi_freelist_tail = NULL;
static Py_ssize_t fi_freelist_len = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I recommend LIFO than FIFO when implementing freelist.
It may be better CPU cache ratio.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to LIFO

@1st1 1st1 merged commit 1b7c11f into python:master Dec 18, 2017
@1st1 1st1 deleted the fastcb branch December 18, 2017 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants