gh-110693: refactor pending calls and use linked list implementation #125567
Conversation
kumaraditya303
force-pushed
the
ll-pending
branch
from
74ac0c7 to
0820a7d
Compare
kumaraditya303
force-pushed
the
ll-pending
branch
from
ce6bd79 to
78c6ee2
Compare
kumaraditya303
requested review from
ericsnowcurrently and
markshannon
as code owners
There was a problem hiding this comment.
Overall I think this is the right idea. There are a few things to consider:
- we still want to keep a statically allocated array for the main pending calls
- we need to be cautious as we switch to an unbounded linked list, and make it easy to dial it back if needed
- consider starting off with a "max" and making it unbounded in a separate PR
- there isn't much reason to change the tests, consider leaving them alone
| /* We keep the number small to preserve as much compatibility | ||
| as possible with earlier versions. */ | ||
| #define MAXPENDINGCALLS_MAIN 32 |
There was a problem hiding this comment.
This point is still valid, so we should probably leave nearly all of the "main" pending calls stuff alone.
| run at once, for the sake of prompt signal handling. This is | ||
| unlikely to cause any problems since there should be very few | ||
| pending calls for the main thread. */ | ||
| #define MAXPENDINGCALLSLOOP_MAIN 0 |
There was a problem hiding this comment.
I'd probably leave this alone, but I'm not opposed to this change.
| /* The maximum allowed number of pending calls. | ||
| If the queue fills up to this point then _PyEval_AddPendingCall() | ||
| will return _Py_ADD_PENDING_FULL. */ | ||
| int32_t max; |
There was a problem hiding this comment.
We should leave this, for the sake of the "main" pending calls stuff.
There was a problem hiding this comment.
It would also be worth leaving it for a while, so we can more easily deal with unexpected consequences of switching to an unbounded linked list.
| A value of 0 means there is no limit (other than the maximum | ||
| size of the list of pending calls). */ | ||
| int32_t maxloop; | ||
| struct _pending_call calls[PENDINGCALLSARRAYSIZE]; |
There was a problem hiding this comment.
We should leave this statically allocated array for the sake of the "main" pending calls. We could go back to the old limit of MAXPENDINGCALLS_MAIN though.
Of course, we'd still use this for per-interpreter pending calls, since it's already there.
| // For example, we use a preallocated array | ||
| // for the list of pending calls. |
There was a problem hiding this comment.
This point is still valid.
2 participants
Uh oh!
There was an error while loading. Please reload this page.