Bug 194665 – [Web GPU] Buffer updates part 1: async mapping functions, unmap, and destroy

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Justin Fan]

[WebGPU] (WIP) Buffer updates part 1: async mapping functions, unmap, and destroy

[Justin Fan]

Created attachment 362052 [details]
Patch

[Justin Fan]

Draft ready for comments but not real review (most tests haven't been updated)

[EWS Watchlist]

Attachment 362052 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

Comment on attachment 362052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362052&action=review

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:73
> +    m_buffer = nullptr;

Is this enough for us to signify destroyed state?

> Source/WebCore/platform/graphics/gpu/GPUBufferUsage.h:38
> +    enum Flags : GPUBufferUsageFlags {

Can't bitwise OR enum class values without casting. Perhaps this should be replaced by a bunch of static consts?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:131
> +// FIXME: MTLStorageModeShared buffers are always mapped? Or should m_mapping not be updated until buffer is unmapped?

BufferOperations.md: "Mappings created with mapWriteAsync are filled with zeros. When they are detached, it is as if buffer.setSubData(0, mapping) was called." Unmap == detach?

[Justin Fan]

Comment on attachment 362052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362052&action=review

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:84
> +    if (descriptor.usage & (GPUBufferUsage::Flags::MapWrite | GPUBufferUsage::Flags::MapRead))

According to the OG proposal back in December MAP_WRITE and MAP_READ are mutually exclusive; is this still true?

[EWS Watchlist]

Comment on attachment 362052 [details]
Patch

Attachment 362052 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11150521

New failing tests:
webgpu/buffer-resource-triangles.html
webgpu/vertex-buffer-triangle-strip.html
webgpu/depth-enabled-triangle-strip.html

[EWS Watchlist]

Created attachment 362062 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Justin Fan]

Comment on attachment 362052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362052&action=review

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:98
>  GPUBuffer::~GPUBuffer()

Should this invalidate the ArrayBuffer memory before returning? What if this GPUBuffer is being destroyed, but a promise with its ArrayBuffer has resolved but not yet delivered to JavaScript?

[Justin Fan]

Created attachment 362185 [details]
Patch

[EWS Watchlist]

Attachment 362185 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:120:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

Created attachment 362187 [details]
Patch

[Justin Fan]

Created attachment 362189 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/48130821>

[Justin Fan]

Remaining questions and issues to be solved for this patch:

1) The design guidelines mention that arrayBuffers provided to promises should reference their buffers to prevent their garbage collection. Is this sufficiently accomplished by capturing the Ref of the buffer in the microtask function?
2) Is a map__Async() call considered "complete" upon returning the promise, or settling the promise?
3) Should a queued map__Async() promise reject if it was created while the buffer was unmapped, but is in mapped state when the callback executes?
4) During unmap(), how do I "neuter" the ArrayBuffer without destroying its contents so that later a mapReadAsync() callback can re-map and read its previous contents?
5) How do I prevent the JS developer from writing to the arraybuffer provided to a mapReadAsync() callback? (Just const-qualify the C++ object? Provide a pointer to a separate copy of the data?)
6) How do I tell when the GPU is done with shared memory? According to the design guidelines, mapWriteAsync() should zero-out its arraybuffer before providing it to the callback, but this seems to happen frequently in the middle of draw calls.

[EWS Watchlist]

Comment on attachment 362189 [details]
Patch

Attachment 362189 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11167323

New failing tests:
webgpu/buffers.html

[EWS Watchlist]

Created attachment 362198 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Justin Fan]

Created attachment 362572 [details]
Patch

[EWS Watchlist]

Attachment 362572 [details] did not pass style-queue:


ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac/TestExpectations:1788:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Justin Fan]

Created attachment 362667 [details]
Patch

[Dean Jackson]

Comment on attachment 362667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362667&action=review

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.h:41
> +    Optional<GPUBindGroupDescriptor> validateAndConvertToGPUVersion() const;

I think this should be named asGPUBindGroupDescriptor()

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:63
> +    unmap();

Will it be an error to call unmap on an already unmapped buffer?

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:68
> +void WebGPUBuffer::rejectOrPendMappingPromise(ArrayBufferPromise&& promise, bool isRead)

I don't like this name. /me looks around for existing examples....

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:76
> +    m_buffer->pendMappingPromise(WTFMove(promise), isRead);

I wonder if it is worth doing more testing here... probably not if GPUBuffer does all the smart stuff (like test if it is already mapped).

> Source/WebCore/platform/graphics/gpu/GPUBuffer.h:48
> +using ArrayBufferPromise = DOMPromiseDeferred<IDLInterface<JSC::ArrayBuffer*>>;

We can't do this. DOMPromiseDeferred lives in bindings/js, and we're inside /platform/.

So we can't use Promises at this level. Instead, they'll be held at the WebGPU level. Down here we'll have to implement our own callbacks.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:131
> +    MicrotaskQueue::mainThreadQueue().append(std::make_unique<VoidMicrotask>([this, protectedThis = makeRef(*this)] () mutable {

Similarly, microtasks are part of the DOM. We'll have to use another queue or make our own. I wonder if Media has anything like this... it must.

[Justin Fan]

Comment on attachment 362667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362667&action=review

>> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:63
>> +    unmap();
> 
> Will it be an error to call unmap on an already unmapped buffer?

According to Corentin's design doc, no.

>> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:68
>> +void WebGPUBuffer::rejectOrPendMappingPromise(ArrayBufferPromise&& promise, bool isRead)
> 
> I don't like this name. /me looks around for existing examples....

registerMappingPromise? ¯\_(ツ)_/¯ We might not need this after I fix the design.

>> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:76
>> +    m_buffer->pendMappingPromise(WTFMove(promise), isRead);
> 
> I wonder if it is worth doing more testing here... probably not if GPUBuffer does all the smart stuff (like test if it is already mapped).

Correct, GPUBuffer tracks state and flags so it handles the checking. Should WebGPUBuffer be tracking that state instead?

[Justin Fan]

Created attachment 362786 [details]
Patch

[Justin Fan]

Created attachment 362792 [details]
Patch

[Dean Jackson]

Comment on attachment 362792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362792&action=review

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:42
> +#if LOG_DISABLED
> +    UNUSED_PARAM(functionName);
> +#endif

You probably don't really need the #if here because the compiler will optimise away the void statement. But whatever.

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:104
> +            if (view)
> +                return static_cast<GPUBindingResource>(view->texture());
> +            return WTF::nullopt;
> +        }, [&layoutBinding, functionName] (const WebGPUBufferBinding& binding) -> Optional<GPUBindingResource> {
> +            if (binding.buffer && binding.buffer->buffer()) {
> +                if (!validateBufferBindingType(binding.buffer->buffer().get(), layoutBinding, functionName))
> +                    return WTF::nullopt;
> +
> +                return static_cast<GPUBindingResource>(GPUBufferBinding { binding.buffer->buffer().releaseNonNull(), binding.offset, binding.size });
> +            }
> +            LOG(WebGPU, "%s: Invalid GPUBindingResource for binding %lu in GPUBindGroupBindings!", functionName, layoutBinding.binding);
> +            return WTF::nullopt;

I suggest flipping the logic in these two functions so the error case comes first. e.g.

if (!view)
  return WTF::nullopt;

return static_cast<GPUBindingResource>(view->texture());

> Source/WebCore/Modules/webgpu/WebGPUBindGroupDescriptor.cpp:111
> +        if (bindingResource)
> +            bindGroupBindings.uncheckedAppend(GPUBindGroupBinding { binding.binding, WTFMove(bindingResource.value()) });
> +        else
> +            return WTF::nullopt;

Same here.

> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:87
> +    m_buffer->registerMappingCallback([promise = WTFMove(promise)] (JSC::ArrayBuffer* arrayBuffer) mutable {
> +        if (!arrayBuffer) {
> +            promise.reject();
> +            return;
> +        }
> +
> +        promise.resolve(arrayBuffer);
> +    }, isRead);

Question: Since we are not keeping a reference to the promise anywhere but in this closure, can we guarantee that this callback will always be called? i.e. if we call a combination of mapReadAsync mapWriteAsync unmap destroy, will the buffer always callback?

> Source/WebCore/Modules/webgpu/WebGPUBuffer.h:44
> +using ArrayBufferPromise = DOMPromiseDeferred<IDLInterface<JSC::ArrayBuffer*>>;

Suggestion: maybe this should be BufferReadOrWritePromise? It's not a promise on the ArrayBuffer.
Looking through WebCore, most cases like this are named FooPromise, with Foo being related to the call that takes the promise.

Also, should the using be inside the class? I think it should.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:119
> +    ASSERT(!m_pendingCallback && !m_mappingCallbackTask.hasPendingTask());
> +

I think we should check here if the m_pendingCallback exists, and if it does.... hmmm... reject it and the new one?

Or does the logic above already handle this? Should we also have a generic test for isMappable()?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:131
> +            if (m_isMapReadable)
> +                m_pendingCallback->callback(stagingBufferForRead());
> +            else
> +                m_pendingCallback->callback(stagingBufferForWrite());

How about a ternary here?

m_pendingCallback->callback(m_isMapReadable ? stagingBufferForRead() : stagingBufferForWrite());

> LayoutTests/webgpu/map-write-buffers.html:70
> +        const promise = buffer.mapWriteAsync();
> +        buffer.unmap();
> +
> +        await promise.then(() => {

Don't we have to check if the promise has been resolved instantly? Or does .then() still execute in that case?

[Justin Fan]

Comment on attachment 362792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362792&action=review

>> Source/WebCore/Modules/webgpu/WebGPUBuffer.cpp:87
>> +    }, isRead);
> 
> Question: Since we are not keeping a reference to the promise anywhere but in this closure, can we guarantee that this callback will always be called? i.e. if we call a combination of mapReadAsync mapWriteAsync unmap destroy, will the buffer always callback?

Hmm. The unmap() function calls the closure (to reject the promise) if it exists, and nowhere else in my code do I destroy the closure. unmap is called by destroy() and ~GPUBuffer() as well, so it is robust. 

I'm not sure exactly what your example means (since a buffer is either map readable or map writeable, not both) but assuming they're both mapReadAsync calls, the second call would immediately reject without registering a callback since the buffer is already mapped by the first. Unmap() first cancels the request's DeferredTask, then calls callback(nullptr) to reject the promise and invalidates the object owning the closure. Destroy() invalidates the MTLBuffer, but at this point the promise has already been rejected.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUBufferMetal.mm:119
>> +
> 
> I think we should check here if the m_pendingCallback exists, and if it does.... hmmm... reject it and the new one?
> 
> Or does the logic above already handle this? Should we also have a generic test for isMappable()?

The existence of m_pendingCallback indicates that the buffer is in the mapped state. The logic above will prevent additional mapping requests on a buffer that already has one. The checks above reject the new one, but leaves m_pendingCallback alone.

the isMap*able() functions actually mean "isUnmapped && wasCreatedWithMap*Flag". I'm renaming m_isMap*able to m_isMap* to match the usage flag that it's based on and to separate it from isMap*able().

>> LayoutTests/webgpu/map-write-buffers.html:70
>> +        await promise.then(() => {
> 
> Don't we have to check if the promise has been resolved instantly? Or does .then() still execute in that case?

Yes, (afaik) .then()'s resolve callback is queued on the task queue immediately if called on a resolved promise.

[Justin Fan]

Created attachment 362836 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362836 [details]
Patch for landing

Clearing flags on attachment: 362836

Committed r241996: <https://trac.webkit.org/changeset/241996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It appears that the changes in https://trac.webkit.org/changeset/241996/webkit

caused these three tests to become flakey:

webgpu/vertex-buffer-triangle-strip.html 
webgpu/depth-enabled-triangle-strip.html 
webgpu/buffer-resource-triangles.html

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgpu%2Fvertex-buffer-triangle-strip.html%20webgpu%2Fdepth-enabled-triangle-strip.html%20webgpu%2Fbuffer-resource-triangles.html

[Justin Fan]

(In reply to Truitt Savell from comment #29)
> It appears that the changes in
> https://trac.webkit.org/changeset/241996/webkit
> 
> caused these three tests to become flakey:
> 
> webgpu/vertex-buffer-triangle-strip.html 
> webgpu/depth-enabled-triangle-strip.html 
> webgpu/buffer-resource-triangles.html
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=webgpu%2Fvertex-buffer-triangle-strip.
> html%20webgpu%2Fdepth-enabled-triangle-strip.html%20webgpu%2Fbuffer-resource-
> triangles.html

It’s probably a race between the draw call and the mapWriteAsync/unmap calls used to fill the vertex buffers for these tests. For buffer updates part 2 (later today) I’m using a non-racy setSubData call that will be implemented in that patch.