Skip to content

[Web] Batch GPU-to-GPU copy commands and add tests for command batching - #20059

Open
akaashrp wants to merge 2 commits into
apache:mainfrom
akaashrp:prereq/webgpu-copy-batching
Open

[Web] Batch GPU-to-GPU copy commands and add tests for command batching#20059
akaashrp wants to merge 2 commits into
apache:mainfrom
akaashrp:prereq/webgpu-copy-batching

Conversation

@akaashrp

@akaashrp akaashrp commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Extend existing batching behavior to avoid flushing after GPU-to-GPU copies. This is valid since the WebGPU spec explicitly describes copyBufferToBuffer as a buffered operation (see https://www.w3.org/TR/webgpu/#buffer-copies). Add WebGPU device mocking and tests covering flushing and ordering behavior in the context of WebGPU command batching.

The WebGPU unit test imports web/src/webgpu.ts directly because WebGPUContext is not exported from the public package entry point. The test-specific TypeScript transformer is only used to load that internal source for this white-box test.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@akaashrp
akaashrp requested a review from guan404ming July 28, 2026 03:05

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

Look nice, left some comments.

web/src/webgpu.ts:531 (outside the diff): could we flush in drawImageFromBuffer too? It submits its own encoder and may render stale data.

Comment thread web/src/webgpu.ts
// A compute submission is now the last queue operation, so the
// A command submission is now the last queue operation, so the
// GPU→CPU copy fast path in sync() is no longer valid.
this.pendingGPUToCPUCopy = null;

@guan404ming guan404ming Jul 28, 2026

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.

Nice cleanup! One catch: flushing nulls the readback promise, so sync() no longer awaits it.

Comment thread web/src/webgpu.ts
* - CPU→GPU writes (deviceCopyToGPU, copyRawBytesToBuffer)
* - GPU↔GPU copies (deviceCopyWithinGPU)
* - Buffer deallocation (deviceFreeDataSpace)
* - Queue sync (sync)

@guan404ming guan404ming Jul 28, 2026

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.

Minor: worth adding drawImageFromBuffer to the "Must be called before" list as well.


expect(queue.submit).toHaveBeenCalledTimes(1);
expect(events).toEqual(["copy", "finish", "submit", "destroy"]);
});

@guan404ming guan404ming Jul 28, 2026

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.

Would you mind adding a case interleaving deviceCopyFromGPU, deviceCopyWithinGPU, then sync()?

* specific language governing permissions and limitations
* under the License.
*/
const { WebGPUContext } = require("../../src/webgpu");

@guan404ming guan404ming Jul 28, 2026

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.

Curious about importing src directly - other node tests use ../../dist. Maybe note the reason?

Comment thread web/jest.config.js
module.exports = {
testEnvironment: "node",

transform: {

@guan404ming guan404ming Jul 28, 2026

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.

Small note: transform replaces the default map, so .js loses babel-jest here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants