Skip to content

fix(core-remote)!: Opt Global out of Send and Sync - #10180

Open
ErichDonGubler wants to merge 4 commits into
gfx-rs:trunkfrom
erichdongubler-mozilla:global-not-impl-send-or-sync
Open

ErichDonGubler wants to merge 4 commits into
gfx-rs:trunkfrom
erichdongubler-mozilla:global-not-impl-send-or-sync

Conversation

@ErichDonGubler

Copy link
Copy Markdown
Member

Connections

Description

Now that Global is only intended to be consumed by downstreams who will be using from a single thread, we'd like to be conservative with what we promise for it. Opt out of Send and Sync for Global by adding a PhantomData<*const ()> member for it.

N.B. that this also includes the replacement of an NBSP, lifted from @nazar-pc's contribution in #9953 (which ended up being retarget to v30 instead of trunk).

Testing

An according assert_not_impl_any!(…) call has been added to wgpu-core-remote.

Squash or Rebase? Rebase.

Checklist

  • I self-reviewed and fully understand this PR.
  • WebGPU implementations built with wgpu may be affected behaviorally. Not at all.
  • Validation and feature gates are in place to confine behavioral changes. None needed.
  • Tests demonstrate the validation and altered logic works.
  • CHANGELOG.md entries for the user-facing effects of this change are present. Typical wgpu downstreams won't use Global any more, so I didn't add an entry.
  • The PR is minimal, and doesn't make sense to land as multiple PRs.
  • Commits are logically scoped and individually reviewable.
  • The PR description has enough context to understand the motivation and solution implemented.

@sagudev sagudev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Now that Global is only intended to be consumed by downstreams who will be using from a single thread, we'd like to be conservative with what we promise for it. Opt out of Send and Sync for Global by adding a PhantomData<*const ()> member for it.

I understand why one would want assert_impl_any!(_: Send, Sync) (to ensure we do not break anything if someone relied on it). But I fail to see why anybody would want to conserve !Send and !Sync. Currently it just allows us to make more optimal code, but that does not need to hold true in the future. I do not see any harm if Global would get Send&Sync back "by accident" in the future.

Comment thread wgpu-core/src/lib.rs

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are these changes intended to be here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, because we should no longer need them. I suppose I should break this out into a separate commit, explaining why it's relevant. Will do that shortly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, actually, maybe these hunks should move to #10177? If that lands first, then I'll keep things here. If not, maybe it'd make sense to roll them into #10177 instead (CC @kpreid).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After a force-push, these hunks have been broken out to ee6b929 (CC @nazar-pc, who is credited with Co-Authored-By).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, actually, maybe these hunks should move to #10177?

That would make more sense to me rather than having them here as this PR does nothing wgc/wgpu otherwise.

@ErichDonGubler

Copy link
Copy Markdown
Member Author

… I fail to see why anybody would want to conserve !Send and !Sync. Currently it just allows us to make more optimal code, but that does not need to hold true in the future. I do not see any harm if Global would get Send&Sync back "by accident" in the future.

I agree that the question of whether Global is Send + Sync or not is not particularly consequential. However, leaving it to be determined by the Rust compiler has exposed a few perf. problems for the new resolver, which was the motivation of #9953 (which is linked in the OP). We can avoid these problems by committing Global to a stance. This doesn't mean that the specific stance is important, though; if we determine that we really do need it to be Send + Sync again, then we enforcing it a la #10177 should be straightforward.

@ErichDonGubler
ErichDonGubler force-pushed the global-not-impl-send-or-sync branch from 856f975 to acca0e2 Compare August 27, 2026 11:09
@sagudev

sagudev commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Aha, so this is some kind of future proofing. If in the future it would become Send/Sync again we would also be close to recursion limit again.

@ErichDonGubler
ErichDonGubler force-pushed the global-not-impl-send-or-sync branch 2 times, most recently from f809654 to 6be84b7 Compare August 27, 2026 11:15
@ErichDonGubler

Copy link
Copy Markdown
Member Author

Rebased to resolve conflicts.

@ErichDonGubler
ErichDonGubler force-pushed the global-not-impl-send-or-sync branch from 6be84b7 to 4630438 Compare August 27, 2026 11:24

@cwfitzgerald cwfitzgerald 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.

One note that CI is mad about, but then LGTM.

Comment thread wgpu-core-remote/src/global/mod.rs Outdated
@github-project-automation github-project-automation Bot moved this to To triage in Triage Aug 31, 2026
@cwfitzgerald cwfitzgerald self-assigned this Sep 2, 2026
@cwfitzgerald cwfitzgerald moved this from To triage to Small in Triage Sep 2, 2026
@ErichDonGubler
ErichDonGubler force-pushed the global-not-impl-send-or-sync branch from 4630438 to fa509e0 Compare September 11, 2026 11:49
ErichDonGubler and others added 4 commits September 11, 2026 14:09
Now that we force the compiler to eagerly evaluate `Send + Sync` for
types that have had problems with `recursion_limit`s, we should be able
to use the compiler's defaults. Remove the attributes raising the
default.

Co-authored-by: Nazar Mokrynskyi <nazar@mokrynskyi.com>
@ErichDonGubler
ErichDonGubler force-pushed the global-not-impl-send-or-sync branch from fa509e0 to 8f774a7 Compare September 11, 2026 12:09
@ErichDonGubler
ErichDonGubler enabled auto-merge (rebase) September 11, 2026 12:09
@ErichDonGubler

Copy link
Copy Markdown
Member Author

I'm not sure why Miri is failing now. This should only simplify the amount of work the compiler has to do. I wonder what I'm missing…and wish I could prioritize this sort of change this week. 😩

@kpreid

kpreid commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

The compiler still has to try evaluating the auto trait requirement.

Does it make any difference if the _prevent_send_and_sync field is first instead of last? That will cut off the check sooner if field order is also traversal order.

@andyleiserson

Copy link
Copy Markdown
Contributor

The error now is on a different type:

error: overflow evaluating the requirement `backend::wgpu_core::CoreCommandEncoder: dispatch::CommonTraits`
    --> wgpu/src/backend/wgpu_core.rs:1795:44
     |
1795 | impl dispatch::CommandEncoderInterface for CoreCommandEncoder {
     |                                            ^^^^^^^^^^^^^^^^^^

This one I think showed up later, not at the same time that we started having problems (could be due to further compiler changes, could be due to our type becoming more complex), and unlike Global, it is still Send/Sync. In #10225 on the v30 branch, I added manual send/sync for this one (well, for a type somewhere in the hierarchy that makes up a CoreComandEncoder; in some cases I made the fix on a different type than the one in the error message to try and more evenly break down the complex nested structures, can't remember if this was such a case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Small

Development

Successfully merging this pull request may close these issues.

5 participants