Summary: | [Web GPU] Buffer updates part 1: async mapping functions, unmap, and destroy | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||||||||||||||||||||
Component: | WebGPU | Assignee: | 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
Justin Fan
2019-02-14 12:56:51 PST
Created attachment 362052 [details]
Patch
Draft ready for comments but not real review (most tests haven't been updated) 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 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 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 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 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 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? Created attachment 362185 [details]
Patch
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.
Created attachment 362187 [details]
Patch
Created attachment 362189 [details]
Patch
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 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 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
Created attachment 362572 [details]
Patch
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.
Created attachment 362667 [details]
Patch
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 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? Created attachment 362786 [details]
Patch
Created attachment 362792 [details]
Patch
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 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. Created attachment 362836 [details]
Patch for landing
Comment on attachment 362836 [details] Patch for landing Clearing flags on attachment: 362836 Committed r241996: <https://trac.webkit.org/changeset/241996> All reviewed patches have been landed. Closing bug. 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 (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. |