FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-74028: `concurrent.futures.Executor.map`: introduce `buffersize` param for lazier behavior by ebonnal · Pull Request #125663 · python/cpython · GitHub

/ cpython Public

gh-74028: concurrent.futures.Executor.map: introduce buffersize param for lazier behavior - #125663

Merged
picnixz merged 45 commits into
python:mainfrom
ebonnal:fix-issue-29842
Mar 13, 2025
ConversationCommits (45)ChecksFiles changed
Merged

gh-74028: concurrent.futures.Executor.map: introduce buffersize param for lazier behavior#125663
picnixz merged 45 commits into
python:mainfrom
ebonnal:fix-issue-29842

Conversation

ebonnal commented Oct 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Context recap (#74028)

Let's consider that we have an input iterable and N = len(iterable).

Current concurrent.futures.Executor.map is $O(N)$ in space (unecessarily expensive on large iterables, completely impossible to use on infinite iterables):
The call results: Iterator = executor.map(func, iterable) iterates over all the elements of the iterable, submitting $N$ tasks to the executor (futures collected into a list of size $N$). Following calls to next(results) take the oldest future from the list (FIFO), then wait for its result and return it.

Proposal: add an optional buffersize param

With this proposal, the call results: Iterator = executor.map(func, iterable, buffersize=b) will iterate only over the first $b$ elements of iterable, submitting $b$ tasks to the executor (futures stored in the buffer deque) and then will return the results iterator.

Calls to next(results) will get the next input element from iterable and submit a task to the executor for it (enqueuing another future), then wait for the oldest future in the buffer queue to complete (FIFO), then return the result.

Benefits:

  • The space complexity becomes $O(b)$
  • When using a buffersize the client code takes back the control over the speed of iteration over the input iterable: after an initial spike of $b$ calls to func to fill the buffer, the iteration over input iterable will follow the rate of the iteration over the results (controlled by the client), which is critical when func involves talking to services that you don't want to overload.

Why a new PR

It turns out it is very similar to the initial work of @MojoVampire in #707 back in 2017 (followed up by @graingert in #18566 and @Jason-Y-Z in #114975): use a queue of fixed size to hold the not-yet-yielded future results.

In addition this PR:

  • uses the intuitive term "buffer"
  • decouple the buffer size from the number of workers
  • unaltered default behavior: keeps the exact same current list-based behavior when buffersize=None (default)
  • integrates concisely into existing logic

📚 Documentation preview 📚: https://cpython-previews--125663.org.readthedocs.build/

ghost commented Oct 17, 2024 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

bedevere-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app Bot mentioned this pull request Oct 17, 2024
Closed
This was referenced Oct 17, 2024
Closed
Closed

bedevere-app Bot commented Oct 17, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

rruuaanng reviewed Oct 18, 2024
View reviewed changes
Comment thread Lib/test/test_concurrent_futures/test_pool.py Outdated

bedevere-app Bot commented Oct 18, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

1 similar comment

bedevere-app Bot commented Oct 18, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

bedevere-app Bot commented Oct 18, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

ebonnal requested a review from rruuaanng October 18, 2024 10:06

bedevere-app Bot commented Oct 18, 2024

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Zheaoli commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

Thanks for the PR.

First, I think this is a big behavior change for Executor. I think we need to discuss it in the https://discuss.python.org/ first.

In my personal opinion, I think this is not a good choice to add the buffersize argument to the api. For now, the API design is based on the original map API. I think this argument will bring more inconsistent into the codebase. And BTW, even if we need the buffersize argument, I think it's not reasonable to forbids the usage of both timeout and buffersize at the same time

The returned iterator raises a TimeoutError if next() is called and the result isn’t available after timeout seconds from the original call to Executor.map().

ebonnal force-pushed the fix-issue-29842 branch 2 times, most recently from 9eef605 to e5c867a Compare October 18, 2024 13:49

ebonnal commented Oct 18, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @Zheaoli, thank you for your comment!

First, I think this is a big behavior change for Executor.

You mean big alternative behavior, right? (the default behavior when ommitting buffersize remaining unchanged)

I think we need to discuss it in the https://discuss.python.org/ first.

Fair, I will start a thread there and ping you.

For now, the API design is based on the original map API. I think this argument will bring more inconsistent into the codebase.

I'm not sure to get it, could you detail that point? 🙏🏻

I think it's not reasonable to forbids the usage of both timeout and buffersize at the same time

You are completely right, makes more sense! I have fixed that (commit)

Zheaoli commented Oct 19, 2024

Copy link
Copy Markdown
Contributor

I'm not sure to get it, could you detail that point? 🙏🏻

For me, the basic map API's behavior is when we put an infinite iterator, the result would be infinite and only stop when the iterator has been stoped. I think we need to keep the same behavior between map and executor.map

ebonnal commented Oct 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @Zheaoli

For me, the basic map API's behavior is when we put an infinite iterator, the result would be infinite and only stop when the iterator has been stoped. I think we need to keep the same behavior between map and executor.map

There may be a misunderstanding here, the goal of this PR is precisely to make Executor.map closer to the builtin map behavior, i.e. make it lazier. (map and current executor.map do not have the same behavior)

I will recap the behaviors so that everybody is on the same page:

built-in map

infinite_iterator = itertools.count(0)

# a `map` instance is created and the func and iterable are just stored as attributes
mapped_iterator = map(str, infinite_iterator)

# retrieves the first element of its input iterator, applies
# the transformation and returns the result
assert next(mapped_iterator) == "0" 

# the next element in the input iterator is the 2nd
assert next(infinite_iterator) == 1

# one can next infinitely
assert next(mapped_iterator) == "2"
assert next(mapped_iterator) == "3" 
assert next(mapped_iterator) == "4" 
assert next(mapped_iterator) == "5" 
...

Executor.map without buffersize (= current Executor.map)

infinite_iterator = itertools.count(0)

# this line runs FOREVER, trying to iterate over input iterator until exhaustion
mapped_iterator = executor.map(str, infinite_iterator)

⏫ this line will run forever because it collects the entire input iterable eagerly, in order to build the entire future results list fs = [self.submit(fn, *args) for args in zip(*iterables)] which requires infinite time and memory.

Executor.map with buffersize

infinite_iterator = itertools.count(0)

# retrieves the first 2 elements (=buffersize) and submits 2 tasks for them
mapped_iterator = executor.map(str, infinite_iterator, buffersize=2)

# retrieves the 3rd element of input iterator and submits a task for it,
# then wait for the oldest future in the buffer to complete and returns the result
assert next(mapped_iterator) == "0" 

# the next element of the input iterator is the 4th
assert next(infinite_iterator) == 3

# one can next infinitely while only a buffer of finite not-yet-yielded future results is kept in memory
assert next(mapped_iterator) == "1" 
assert next(mapped_iterator) == "2" 
assert next(mapped_iterator) == "4"
assert next(mapped_iterator) == "5" 
...

note

I used the example of an infinite input iterator because this is an example where current Executor.map is just unusable at all. But even for finite input iterables, if a developer writes mapped_iterator = executor.map(fn, iterable), they often don’t want the iterable to be eagerly exhausted right away, but rather to be iterated at the same rate as mapped_iterator. This PR's proposal is to allow them to do so by setting a buffersize.

ebonnal commented Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

hey @rruuaanng, fyi I have applied your requested changes regarding the integration of unit tests into existing class 🙏🏻

rruuaanng reviewed Oct 25, 2024
View reviewed changes
Comment thread Lib/concurrent/futures/_base.py
Comment thread Lib/concurrent/futures/_base.py Outdated
args_iter = iter(zip(*iterables))
if buffersize:
fs = collections.deque(
self.submit(fn, *args) for args in islice(args_iter, buffersize)

rruuaanng Oct 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Isn't buffersize empty? Can you introduce it? (Forgive me for not understanding it).

ebonnal Oct 25, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

absolutely np, thank you for taking the time to review my proposal. To be sure to understand the question well, what do you mean by "Isn't buffersize empty?"

ebonnal Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hey @rruuaanng , I have reworked the PR's description, I hope it makes things clearer!

ebonnal commented Oct 28, 2024

Copy link
Copy Markdown
Contributor Author

Hey @NewUserHa @AA-Turner @serhiy-storchaka, this may interest you given your recent activity on #14221 🙏🏻

ebonnal requested a review from rruuaanng November 16, 2024 22:36
picnixz reviewed Dec 3, 2024
View reviewed changes
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Lib/concurrent/futures/_base.py
Comment thread Lib/concurrent/futures/_base.py
Comment thread Misc/NEWS.d/next/Library/2024-10-18-10-27-54.gh-issue-74028.4d4vVD.rst Outdated
Comment thread Lib/concurrent/futures/_base.py
Comment thread Lib/concurrent/futures/_base.py Outdated
if (
buffersize
and (executor := executor_weakref())
and (args := next(args_iter, None))

picnixz Dec 3, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

args may be empty, so you need to check for args is not None

ebonnal Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are you refering to the case where one call executor.map(func) without any input iterable?

picnixz Dec 3, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. You can't always assume that func needs an input (or do you?)

ebonnal Dec 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

you are right! But in such a case we don't enter the while fs: (fs being empty in that case), right?

ebonnal Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@picnixz I have added unit tests checking the behavior with multiple input iterables and without any input iterables.

picnixz Dec 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

But in such a case we don't enter the while fs.

Not necessarily. What I meant is that you call executor.map with an input iterable that yields args = () everytime.

Note that it also doesn't hurt to check is not None because it's probably slightly faster since otherwise you need to call __bool__ on the args being yielded.

ebonnal Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So for example a call like executor.map(func, [()])? In such a call we get iterables = ([()],) and args_iter = iter(zip(*([()],))) and next(args_iter,) will be ((),) (not ()). You may have missed the ziping in your reasoning?

In term of pure readability of the code I struggle to have an opinion, do you feel that (args := next(args_iter, None)) is not None is more natural?

picnixz Dec 5, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You may have missed the ziping in your reasoning?

I did :) Sorry, my bad!

do you feel that (args := next(args_iter, None)) is not None is more natural?

I feel it would at least help avoiding questions like mine! (and it would still be probably slightly better performance wise but this claim is just my gut feeling).

ebonnal Dec 5, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@picnixz oh yes I see... I have renamed args_iter into a more self-explanatory zipped_iterables, do you think it would be enough to avoid the confusion?

(Because I am scared that the addition of is not None may misslead some of our fellow pythonistas wondering "wait, why is this not None check necessary here, what am I missing here 🤔?")

picnixz Dec 13, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Personally, I like having the is not None just so that I don't have to wonder what's args_iter is precisely yielding. I can assume that it's yielding a tuple-like object, but I don't necessarily know the shape of that tuple. So is not None discriminates probable items and the sentinel value. So I'd say it's still pythonic.

Performance-wise it should be roughly the same (one checks that the tuple's size != 0 and the other just compares if it's the None singleton but both are essentially a single comparison).

Now up to you. If others didn't observe (like me) that args_iter never yields an empty tuple, then it's probably better to keep the is not None check for clarity.

ebonnal commented Dec 3, 2024
View reviewed changes

ebonnal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Many thanks for your review @picnixz 🙏🏻 !

Comment thread Lib/concurrent/futures/_base.py
Comment thread Lib/concurrent/futures/_base.py
Comment thread Lib/concurrent/futures/_base.py Outdated
if (
buffersize
and (executor := executor_weakref())
and (args := next(args_iter, None))

ebonnal Dec 3, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Are you refering to the case where one call executor.map(func) without any input iterable?

hugovk commented Jan 12, 2025

Copy link
Copy Markdown
Member

So I have accidentally rebased on an outdated main and then pushed and it somehow triggered a request for review to all the maintainers, sorry everyone for that noise 😬 @picnixz I would greatly appreciate if you can remove the review requests when you are around 🙏🏻 ! Many thanks

Edit: Thanks @AA-Turner for the clean up!

No problem, but please refrain from force pushing. Everything will be squash-merged in the end.

In order to keep the commit history intact, please avoid squashing or amending history and then force-pushing to the PR. Reviewers often want to look at individual commits.

https://devguide.python.org/getting-started/pull-request-lifecycle/#quick-guide

Thank you!

ebonnal commented Jan 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@hugovk ok, will merge main instead of rebasing next time, thanks for the pointer! 🙏🏻

ebonnal commented Jan 25, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hi @gpshead, whenever you get a chance, your feedback on this would be really appreciated 🙏🏻

gpshead reviewed Feb 24, 2025
View reviewed changes

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks great to me. One detail: Lets make buffersize a keyword only argument.

ebonnal commented Feb 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@gpshead Thank you for your review, I’m glad the proposal makes sense to you too 🙏🏻 .

One detail: Lets make buffersize a keyword only argument.

Because they are preceded by *iterables, chunksize/timeout/buffersize are already keyword-only args, or am I missing something?

picnixz commented Feb 24, 2025

Copy link
Copy Markdown
Member

@ebonnal I've introduced conflicts I think due to the rewording of the start-method section. Could you fix them please? TiA

picnixz approved these changes Feb 24, 2025
View reviewed changes

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Except for the conflicts to change, I'm fine with this changeset. We could also do the same for Pool.map for multiprocessing but in a follow-up PR if you want (I don't know if the task will be easy or relevant btw)

ebonnal commented Feb 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@ebonnal I've introduced conflicts I think due to the rewording of the start-method section. Could you fix them please? TiA

Merged main ✅

Except for the conflicts to change, I'm fine with this changeset. We could also do the same for Pool.map for multiprocessing but in a follow-up PR if you want (I don't know if the task will be easy or relevant btw)

@picnixz Thank you for all your support on this PR, I will follow up on the Pool-side.

picnixz reviewed Feb 24, 2025
View reviewed changes
Comment thread Doc/whatsnew/3.14.rst
Update Doc/whatsnew/3.14.rst
ef814e5 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz commented Feb 24, 2025

Copy link
Copy Markdown
Member

I'll rerun the CI tomorrow if it still fails, don't worry

serhiy-storchaka reviewed Mar 3, 2025
View reviewed changes
Comment thread Lib/concurrent/futures/_base.py Outdated
serhiy-storchaka approved these changes Mar 3, 2025
View reviewed changes

serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. 👍

ebonnal mentioned this pull request Mar 10, 2025
Closed
picnixz approved these changes Mar 12, 2025
View reviewed changes

picnixz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the new line I wanted you to add has been gobbled by another commit :(

Comment thread Doc/whatsnew/3.14.rst
add new line in 3.14.rst
bb756f4 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz commented Mar 12, 2025

Copy link
Copy Markdown
Member

I'll merge this tomorrow or on Friday (today is review's day!)

picnixz commented Mar 13, 2025

Copy link
Copy Markdown
Member

Thank you for the contribution!

ebonnal commented Mar 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thank you @picnixz @gpshead @serhiy-storchaka for making this change go through! 🙏🏻

This was referenced Mar 13, 2025
Merged
Open
dalcinl reviewed Apr 28, 2025
View reviewed changes
Comment thread Lib/concurrent/futures/_base.py
# reverse to keep finishing order
fs.reverse()
while fs:
if (

dalcinl Apr 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 of buffsize tasks before, here in this branch you are appending an EXTRA task to the queue, and now you have buffsize + 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:

diff --git a/Lib/concurrent/futures/_base.py b/Lib/concurrent/futures/_base.py
index d98b1ebdd58..3b9ccf4d651 100644
--- a/Lib/concurrent/futures/_base.py
+++ b/Lib/concurrent/futures/_base.py
@@ -628,17 +628,17 @@ def result_iterator():
                 # reverse to keep finishing order
                 fs.reverse()
                 while fs:
+                    # Careful not to keep a reference to the popped future
+                    if timeout is None:
+                        yield _result_or_cancel(fs.pop())
+                    else:
+                        yield _result_or_cancel(fs.pop(), end_time - time.monotonic())
                     if (
                         buffersize
                         and (executor := executor_weakref())
                         and (args := next(zipped_iterables, None))
                     ):
                         fs.appendleft(executor.submit(fn, *args))
-                    # Careful not to keep a reference to the popped future
-                    if timeout is None:
-                        yield _result_or_cancel(fs.pop())
-                    else:
-                        yield _result_or_cancel(fs.pop(), end_time - time.monotonic())
             finally:
                 for future in fs:
                     future.cancel()

ebonnal Apr 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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:

it = executor.map(fn, iterable, buffersize=buffersize)
# point A
next(it)
# point B
next(it)
# point C

"enqueue -> wait -> yield" (current, introduced by this PR) behavior

  • buffers buffersize tasks
  • at # point A there is buffersize buffered tasks
  • 1st call to next:
    • enqueue a task, jumping to buffersize+1 tasks in buffer
    • wait for the next result to be available
    • yield the next result, finally going down to buffersize tasks in buffer
  • at # point B there is still buffersize buffered tasks
  • 2nd call to next: same

pro: buffersize tasks in buffer between two calls to next
con: while waiting for the next result we have buffersize+1 tasks in buffer

"wait -> yield -> enqueue" (your proposal) behavior

  • buffers buffersize tasks
  • at # point A there is buffersize buffered tasks
  • call to next:
    • wait for the next result to be available
    • yield this result, going down to buffersize - 1 tasks in buffer
  • at # point B there is buffersize - 1 buffered tasks
  • call to next:
    • enqueue a task, jumping back to buffersize tasks in buffer
    • wait for the next result to be available
    • yield next result, going down to buffersize - 1 tasks in buffer
  • at # point C there is still buffersize-1 buffered tasks

pro: never exceed buffersize
con: between two calls to next we have only buffersize - 1 tasks in buffer

"wait -> enqueue -> yield" (#131467) behavior

  • buffers buffersize tasks
  • at # point A there is buffersize buffered tasks
  • 1st call to next:
    • wait for next result to be available
    • enqueue a task, jumping to buffersize + 1 tasks in buffer
    • yield already available result without needing to wait, going back to buffersize tasks in buffer
  • at # point B there is still buffersize buffered tasks
  • 2nd call to next: same

pros:

  • buffersize tasks in buffer between two calls to next
  • jumps to buffersize + 1 during a call to next but is back instantly to buffersize 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 🙏🏻

dalcinl mentioned this pull request Apr 28, 2025
Merged
obaltian mentioned this pull request Jul 19, 2025
Closed
obaltian mentioned this pull request Jul 28, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes
picnixz picnixz approved these changes
gpshead Awaiting requested review from gpshead
+2 more reviewers
dalcinl dalcinl left review comments
rruuaanng rruuaanng left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

gpshead

picnixz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL