Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebGPUBackend: Fix Timestamp Query #30299

Merged
merged 2 commits into from
Jan 10, 2025
Merged

WebGPUBackend: Fix Timestamp Query #30299

merged 2 commits into from
Jan 10, 2025

Conversation

ycw
Copy link
Contributor

@ycw ycw commented Jan 9, 2025

descriptor should set timestampWrites upon initTimestampQuery, w/o the prop, timestamps will not be capture in query set, demo the issue: https://jsfiddle.net/4zcayfn3/ (time spent identically per render context)

.resolveTimestampAsync() is currently immediate-resolved, results in info fails to catch up the render, it should await info get updated.

Copy link

github-actions bot commented Jan 9, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 339.51
79.09
339.51
79.09
+0 B
+0 B
WebGPU 491
136.35
490.99
136.33
-12 B
-15 B
WebGPU Nodes 490.46
136.24
490.45
136.22
-12 B
-15 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 465.35
112.14
465.35
112.14
+0 B
+0 B
WebGPU 560.3
151.75
560.28
151.74
-12 B
-13 B
WebGPU Nodes 516.39
141.54
516.38
141.53
-12 B
-13 B

@mrdoob mrdoob added this to the r173 milestone Jan 10, 2025
@mrdoob mrdoob merged commit 5fb35c7 into mrdoob:dev Jan 10, 2025
12 checks passed
@RenaudRohlinger
Copy link
Collaborator

RenaudRohlinger commented Jan 10, 2025

Nice! Actually, on that note, I’m considering reworking the entire logic. The current approach is quite limited and can become unstable, especially when two buffers are allocated per renderContext. Once you exceed 50–100 compute or render programs per frame, many GPUs start to throw errors breaking the app.

Instead, I’m thinking of creating two instances of a new TimestampQueryPool class. Each context would simply append its offsets (beginningOfPassWriteIndex and endOfPassWriteIndex) to the pool. This seems far more stable, allowing to retrieve the full set of timestamp results at once, something like:

const timestamps = await renderer.timestampResolve();
// timestamps is a BigUint64Array
// also updates the Info class internally

This way, we avoid forcing intermediate await calls for each individual render/compute, which currently disrupts the rendering pipeline and hurts performance.

@ycw
Copy link
Contributor Author

ycw commented Jan 10, 2025

you may need multiple pool-pairs because query set count is max to 4096 ...

@ycw ycw deleted the fix-timestamp-query branch January 10, 2025 08:25
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.

4 participants