fix(core): Flush pending writes before mapAsync, if needed

[andyleiserson]

Fixes #5173. Also fixes an open TODO to flush pending writes in on_submitted_work_done.

Based on top of #9439 and #9444. The changes for this PR are the commits after the merge commit.

This implements the strategy @teoxoy suggested in bug 1994733, comment 6: if mapAsync finds that PendingWrites holds writes for the buffer being mapped, then PendingWrites is flushed/submitted before mapping the buffer.

This strategy never makes Queue::writeBuffer or unmap perform a submission. A submission only happens if/when mapAsync is called for a buffer with pending writes. This has the nice property that mapAsync was already an operation that might have to wait on the queue, although this change broadens the circumstances when that will happen and may increase the amount of queued work that mapAsync has to wait on. (More sophisticated approaches would be to only flush the pending writes for the buffer in question, rather than all of them, or to proactively flush pending writes if the queue is idle, to reduce the chance that mapAsync ends up waiting on them.)

There is a long-standing TODO in mapAsync about possibly needing a barrier. I briefly considered trying to address that as part of that change, but I think it's a separable concern, and comes with its own set of performance risks that it is better not to lump together with this change.

Testing
Adds a directed test and some CTS tests.

Squash or Rebase? Rebase

Checklist

• Need to add a CHANGELOG.md entry.

[teoxoy]

Good stuff!

[jimblandy]

If I understand correctly, this patch could invoke user callbacks from Buffer::map_async. This is new user-visible behavior (potentially unexpected, but I bet it'll be okay). There are a few places in the wgpu documentation that explain when callbacks get invoked that should be updated. For example:

• the docs for Buffer::map_async
• the module-level documentation for Buffer
• the docs for Queue::on_submitted_work_done

There may be other spots. i'd look for references to poll.

[jimblandy]

Oh, and obviously, the change in callback behavior needs prominent mention in the CHANGELOG.

[jimblandy]

It is not correct to hoist the snatch lock acquisition outside Queue::submit. The user's callbacks must always be invoked while wgpu-core is holding no locks, but this change lets submit_inner invoke them while its caller holds the Device's snatch lock.

[andyleiserson]

It is not correct to hoist the snatch lock acquisition outside Queue::submit. The user's callbacks must always be invoked while wgpu-core is holding no locks, but this change lets submit_inner invoke them while its caller holds the Device's snatch lock.

It doesn't hold the snatch lock while calling callbacks -- the guard is moved into device.maintain, which drops it. (That was the case before I made any changes, but I did have to study it, since at first glance the lock does appear to be released due to going out of scope.) I added a comment about this on line 1528, but probably a comment on fn submit itself would be appropriate, assuming we don't undertake a bigger refactoring of submit.

[jimblandy]

It doesn't hold the snatch lock while calling callbacks -- the guard is moved into device.maintain, which drops it.

Okay, I was misreading the code. Yes, this isn't a problem after all.

[andyleiserson]

I have a version of this updated based on #9361, but it has a new test failure in webgpu:api,operation,buffers,map:mapAsync,write,* that was not present with the original version.

[teoxoy]

What is the new test failure?

[andyleiserson]

What is the new test failure?

The new test failure was a silly mistake in the refactoring I did of submit_with_pending_writes where I wasn't actually submitting the things I intended to submit 🤦.

[andyleiserson]

In the latest version I've also added a flush of pending writes in on_submitted_work_done (which was noted in a TODO).

[teoxoy]

Looks good, I only have one last concern.

[andyleiserson]

I'm not sure if this is actually needed here. We've added the texture to pending_writes, and submission already scans for surface textures in pending writes.

I'm actually not sure whether we need to have submission.surface_textures at all. Instead of collecting surface textures in validate_command_buffer and transitioning them in submit, we could combine that with the texture transition logic for pending writes in submit_pending_submission. But that would be a bigger refactoring.

[teoxoy]

That does sound right, pending_writes.insert_texture(texture); was added in a later revision of #9361, so I think we can simplify things now.

[andyleiserson]

I am going to address this in #9473.

map_async now uses a refactored submission codepath that does not invoke callbacks.