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

webgpu: Use WGPU poller thread for poll_all_devices #32266

Merged
merged 4 commits into from May 15, 2024

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented May 10, 2024

As discussed https://servo.zulipchat.com/#narrow/stream/263398-general/topic/ipc_channel, firefox will also switch to something similar in the future: https://bugzilla.mozilla.org/show_bug.cgi?id=1870699.

In future we could make thread per device, but that would require hashmap for Pollers.

Fastgame still works: https://sagudev.github.io/briefcase/fastgame.html

try run: https://github.com/sagudev/servo/actions/runs/9051091523/job/24867638272


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes in WebGPU CTS

@sagudev sagudev added the A-content/webgpu The WebGPU implementation. label May 10, 2024
@sagudev
Copy link
Member Author

sagudev commented May 10, 2024

Timings are more sane now (we actually outperform firefox on onSubmittedWorkDone tests due to https://bugzilla.mozilla.org/show_bug.cgi?id=1870699):
Servo run onSubmittedWorkDone tests in 350ms
Firefox run onSubmittedWorkDone tests in 101535.0 ms = 1.7 min

EDIT: According to measurements on my computer we are also faster than chromium (edge) on onSubmittedWorkDone tests (tested edge vs. servo on my win11 machine).

@sagudev

This comment was marked as outdated.

@sagudev

This comment was marked as resolved.

@sagudev
Copy link
Member Author

sagudev commented May 12, 2024

Some flakes that do occur but rarely (sometimes they are stable, other times they simply do not happen):

  • CRASH [expected OK] /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,buffer,mapping:mapAsync,state,destroyed:*
    with:
called `Result::unwrap()` on an `Err` value: Destroyed (thread WGPU poller, at components/webgpu/wgpu_thread.rs:112)

(this is actually our fault as we do not handle this situation).

  • ERROR /_webgpu/webgpu/cts.https.html?q=webgpu:api,validation,buffer,mapping:mapAsync,state,mappingPending:* subtest: TIMEOUT instead of FAIL (I have no idea about this one, but once implemented correctly it should work).
  • TIMEOUT [expected OK] /_webgpu/webgpu/cts.https.html?q=webgpu:api,operation,compute,basic:large_dispatch:* (deadlock in wgpu: [core] deadlock between poll_all_devices and queue_submit gfx-rs/wgpu#5695)

@sagudev sagudev marked this pull request as ready for review May 12, 2024 11:49
Copy link
Member

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

LGTM, with some nits and perhaps one refactoring.

components/webgpu/poll_thread.rs Outdated Show resolved Hide resolved
components/webgpu/poll_thread.rs Show resolved Hide resolved
components/webgpu/poll_thread.rs Outdated Show resolved Hide resolved
@gterzian
Copy link
Member

(this is actually our fault as we do not handle this situation).

File an issue for this one? Seems like we can catch the destroy error and send an error back to script?

@sagudev
Copy link
Member Author

sagudev commented May 13, 2024

(this is actually our fault as we do not handle this situation).

File an issue for this one? Seems like we can catch the destroy error and send an error back to script?

Done #32277

Copy link
Member

@gterzian gterzian left a comment

Choose a reason for hiding this comment

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

LGTM

@sagudev sagudev added this pull request to the merge queue May 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 14, 2024
@sagudev sagudev added this pull request to the merge queue May 15, 2024
Merged via the queue into servo:main with commit 00f267e May 15, 2024
9 checks passed
@sagudev sagudev deleted the wgpu-poller branch May 15, 2024 04:31
@sagudev sagudev restored the wgpu-poller branch May 15, 2024 08:31
sagudev added a commit to sagudev/servo that referenced this pull request May 15, 2024
…bgpu)

{"fail_fast": false, "matrix": [{"name": "WebGPU CTS", "workflow": "linux", "wpt_layout": "2020", "profile": "production", "unit_tests": false, "wpt_tests_to_run": "_webgpu"}]}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/webgpu The WebGPU implementation.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants