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

Remove timeout in awaitable #651

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Remove timeout in awaitable #651

merged 1 commit into from
Dec 9, 2024

Conversation

almarklein
Copy link
Member

Closed #650

cc @fyellin

@almarklein almarklein requested a review from Korijn as a code owner December 9, 2024 12:13
@almarklein almarklein merged commit 926c7cc into main Dec 9, 2024
20 checks passed
@almarklein almarklein deleted the awaitable-timeout branch December 9, 2024 13:37
@fyellin
Copy link
Contributor

fyellin commented Dec 11, 2024

Now that we have eliminated timeouts, I really do hate the fact that we are doing a busy wait. Would it make sense to do a very slow backoff? Maybe call poll() ten times in a row, and only then do we start to sleep, say 1ms between calls to poll().

For synchronous calls, I was wondering if it makes sense for us to use poll_wait instead of poll, but it really doesn't. poll_wait waits until Everything on the queue is done, rather than waiting until just the action we're interested in is done.

@Korijn
Copy link
Collaborator

Korijn commented Dec 11, 2024

It's probably more efficient to invest our time and energy into other things until upstream wgpu-native implements callbacks/futures/promises or something of that nature. Then we can build our polling-free implementation on top.

@almarklein
Copy link
Member Author

almarklein commented Dec 11, 2024

@Korijn I agree that we (hopefully) eventually get a polling-free implementation, but in the mean time, adding a bit of logic to sleep a bit longer than zero seconds sounds like a good idea to me. Apparently in some cases the future may take over 5 s to resolve, and that's a much longer time to busy-wait that we first anticipated.

@fyellin do you have time to create a pr for that?

Maybe something like:

sleep_time = min(0.01, sleep_time + 0.001)

@fyellin
Copy link
Contributor

fyellin commented Dec 11, 2024

Yes. That was pretty much my thoughts exactly. I didn't mind constant polling when we believed the results would always be returned quickly. But now that we realize that polling may take 5s or more, than's not reasonable. I'll come up with something reasonable (and easy to modify if you don't like it) and submit a PR.

@Korijn
Copy link
Collaborator

Korijn commented Dec 11, 2024

It is clear that the use case in #650 is compute shaders right? For real time rendering use cases, results should always be returned quickly.

Regardless, I think both of you are probably right that some kind of back off mechanism is appropriate.

@fyellin
Copy link
Contributor

fyellin commented Dec 11, 2024 via email

@Korijn
Copy link
Collaborator

Korijn commented Dec 11, 2024

I guess you are right! 👍🏻

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.

device.queue.read_buffer times out
3 participants