Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gh-74028:
concurrent.futures.Executor.map
: introducebuffersize
param for lazier behavior #125663New 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
gh-74028:
concurrent.futures.Executor.map
: introducebuffersize
param for lazier behavior #125663Changes from all commits
45c3ec5
bfb2c5c
8539663
022b8c6
7ced787
cb5f26e
f46ebe6
eb26e86
0821f95
d95c55b
c80f466
90e6d7c
ab91694
01b8adf
2f8a63f
1fb53a5
365c85d
bf5f838
a0057f1
1aa1275
e0a9a9e
8d6ea97
6124868
c11276f
ebb5337
602968c
b14e368
d37ce09
cdf239c
0a49784
178d6fe
516a94b
ba4ac81
9588059
0427bf1
af88fdf
1fcf3fe
0892b2b
579ba31
332826a
ef814e5
26c8d8d
7b1d5f6
8dac531
bb756f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebonnal I believe you got this part slightly wrong, "off-by-one". IIUC, the number of pending futures cannot be larger than
buffsize
. However, after the initial submission ofbuffsize
tasks before, here in this branch you are appending an EXTRA task to the queue, and now you havebuffsize + 1
tasks that have potentially not yielded yet.Fortunately, looks like the fix is trivial: you simply have to yield first, next append to the queue:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dalcinl,
TL;DR: fyi we have #131467 that is open and tackling this "off-by-one" situation. Would be great to get your review there! 🙏🏻
I have not proposed this variation at first because I think it makes sense as an optional follow up PR given that it integrates slightly less smoothly into existing logic.
You will notice that it is not "just" moving the yield before the enqueue, let's see why on a simple scenario, explaining the 3 behaviors:
Scenario:
"enqueue -> wait -> yield" (current, introduced by this PR) behavior
buffersize
tasks# point A
there isbuffersize
buffered tasksnext
:buffersize+1
tasks in bufferbuffersize
tasks in buffer# point B
there is stillbuffersize
buffered tasksnext
: samepro:
buffersize
tasks in buffer between two calls tonext
con: while waiting for the next result we have
buffersize+1
tasks in buffer"wait -> yield -> enqueue" (your proposal) behavior
buffersize
tasks# point A
there isbuffersize
buffered tasksnext
:buffersize - 1
tasks in buffer# point B
there isbuffersize - 1
buffered tasksnext
:buffersize
tasks in bufferbuffersize - 1
tasks in buffer# point C
there is stillbuffersize-1
buffered taskspro: never exceed
buffersize
con: between two calls to
next
we have onlybuffersize - 1
tasks in buffer"wait -> enqueue -> yield" (#131467) behavior
buffersize
tasks# point A
there isbuffersize
buffered tasksnext
:buffersize + 1
tasks in bufferbuffersize
tasks in buffer# point B
there is stillbuffersize
buffered tasksnext
: samepros:
buffersize
tasks in buffer between two calls tonext
buffersize + 1
during a call tonext
but is back instantly tobuffersize
because the next result is already available when we enqueued the next task (the closest we can get to a yield-and-enqueue-at-the-same-time).Let me know if it makes sense 🙏🏻