Bug 194665

Summary: [Web GPU] Buffer updates part 1: async mapping functions, unmap, and destroy
Product: WebKit Reporter: Justin Fan <justin_fan>
Component: WebGPUAssignee: Justin Fan <justin_fan>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dino, esprehn+autocc, ews-watchlist, graouts, kondapallykalyan, rniwa, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Justin Fan 2019-02-14 12:56:51 PST
[WebGPU] (WIP) Buffer updates part 1: async mapping functions, unmap, and destroy
Comment 1 Justin Fan 2019-02-14 12:57:36 PST
Created attachment 362052 [details]
Patch
Comment 2 Justin Fan 2019-02-14 12:58:11 PST
Draft ready for comments but not real review (most tests haven't been updated)
Comment 3 EWS Watchlist 2019-02-14 12:59:01 PST
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.
Comment 4 Justin Fan 2019-02-14 13:02:36 PST
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?
Comment 5 Justin Fan 2019-02-14 13:18:48 PST
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?
Comment 6 EWS Watchlist 2019-02-14 14:22:03 PST
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
Comment 7 EWS Watchlist 2019-02-14 14:22:05 PST
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
Comment 8 Justin Fan 2019-02-14 17:08:08 PST
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?
Comment 9 Justin Fan 2019-02-15 16:57:30 PST
Created attachment 362185 [details]
Patch
Comment 10 EWS Watchlist 2019-02-15 16:59:10 PST
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.
Comment 11 Justin Fan 2019-02-15 17:05:25 PST
Created attachment 362187 [details]
Patch
Comment 12 Justin Fan 2019-02-15 17:10:20 PST
Created attachment 362189 [details]
Patch
Comment 13 Radar WebKit Bug Importer 2019-02-15 17:22:24 PST
<rdar://problem/48130821>
Comment 14 Justin Fan 2019-02-15 17:24:44 PST
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.
Comment 15 EWS Watchlist 2019-02-15 18:18:16 PST
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
Comment 16 EWS Watchlist 2019-02-15 18:18:18 PST
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
Comment 17 Justin Fan 2019-02-20 17:02:17 PST
Created attachment 362572 [details]
Patch
Comment 18 EWS Watchlist 2019-02-20 17:04:35 PST
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.
Comment 19 Justin Fan 2019-02-21 16:24:37 PST
Created attachment 362667 [details]
Patch
Comment 20 Dean Jackson 2019-02-21 16:56:23 PST
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.
Comment 21 Justin Fan 2019-02-22 12:05:05 PST
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?
Comment 22 Justin Fan 2019-02-22 16:48:48 PST
Created attachment 362786 [details]
Patch
Comment 23 Justin Fan 2019-02-22 16:57:19 PST
Created attachment 362792 [details]
Patch
Comment 24 Dean Jackson 2019-02-22 17:28:57 PST
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?
Comment 25 Justin Fan 2019-02-23 14:45:42 PST
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.
Comment 26 Justin Fan 2019-02-23 14:45:52 PST
Created attachment 362836 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2019-02-23 15:24:33 PST
Comment on attachment 362836 [details]
Patch for landing

Clearing flags on attachment: 362836

Committed r241996: <https://trac.webkit.org/changeset/241996>
Comment 28 WebKit Commit Bot 2019-02-23 15:24:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Truitt Savell 2019-02-25 13:02:38 PST
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
Comment 30 Justin Fan 2019-02-25 14:00:50 PST
(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.