Replace JSON case cache serialization with binary files #3094

ben-clayton · 2023-10-25T18:22:38Z

This PR removes a the need to create bunch of temporary JSON objects, reducing the amount of garbage collection we need to do.

This change also changes the DataCache to be unbounded to a 4-element LRU cache, capping the amount of memory used.

Issue: #3039

Requirements for PR author:

All missing test coverage is tracked with "TODO" or .unimplemented().
New helpers are /** documented */ and new helper files are found in helper_index.txt.
Test behaves as expected in a WebGPU implementation. (If not passing, explain above.)

Requirements for reviewer sign-off:

Tests are properly located in the test tree.
Test descriptions allow a reader to "read only the test plans and evaluate coverage completeness", and accurately reflect the test code.
Tests provide complete coverage (including validation control cases). Missing coverage MUST be covered by TODOs.
Helpers and types promote readability and maintainability.

When landing this PR, be sure to make any necessary issue status updates.

github-actions · 2023-10-25T20:04:57Z

Previews, as seen when this build job started (d238b9f):
Run tests | View tsdoc

zoddicus

Did a first-pass on this, the broad overview of this looks reasonable to me, I will take another look in the morning.

This is a pretty complex TS change, so adding Kai to the review for more eyes/expertise.

austinEng

honestly much simpler than I expected! which is great

one comment about potentially using DataView so that handling the offsets is likely simpler

also, I see "@ p" in various places - is this supposed to be "@ param" ?
(I've inserted a space as to not tag random folks)

austinEng · 2023-10-25T20:39:29Z

src/webgpu/util/binary_stream.ts

+    this.i32 = new Int32Array(this.u8.buffer);
+    this.f16 = new Float16Array(this.u8.buffer);
+    this.f32 = new Float32Array(this.u8.buffer);
+    this.f64 = new Float64Array(this.u8.buffer);


would it be simpler with DataView?

blog says it should be just as fast as native typed arrays if you use little-endian
https://v8.dev/blog/dataview

Maybe. I briefly looked at DataView but reached for what I knew worked. I don't mind trying DataView as a followup change, to see if there are any perf wins.

src/common/runtime/standalone.ts

ben-clayton · 2023-10-25T20:57:04Z

also, I see "@ p" in various places - is this supposed to be "@ param" ?

@p is doxygen for link-the-next word to the parameter with the given name, where as @param provides the definition for the parameter. VSCode was highlighting @p similarly, so I just assumed it was valid markup for TS. Can change if it's not.

github-actions · 2023-10-25T21:01:20Z

Previews, as seen when this build job started (0651deb):
Run tests | View tsdoc

austinEng · 2023-10-25T21:01:32Z

also, I see "@ p" in various places - is this supposed to be "@ param" ?

@p is doxygen for link-the-next word to the parameter with the given name, where as @param provides the definition for the parameter. VSCode was highlighting @p similarly, so I just assumed it was valid markup for TS. Can change if it's not.

I didn't know this, but fine with me

kainino0x · 2023-10-25T23:31:04Z

I'll defer to Austin's review unless someone specifically wants mine.

kainino0x · 2023-10-25T23:33:21Z

Unsure if @p works in tsdoc, you can check the generated documentation if you want to be sure (but it doesn't really matter that much). Also the mouseover-popup in vscode will render the documentation for you.

Once gpuweb/cts#3094 lands and rolls, the case cache will move from JSON to binary. The fetch() needs to handle this change of encoding. Change-Id: Iecc8ead12bdff2d64bc3bf129022bf4686ce70d9 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/158061 Commit-Queue: Ben Clayton <[email protected]> Auto-Submit: Ben Clayton <[email protected]> Reviewed-by: Austin Eng <[email protected]> Commit-Queue: Austin Eng <[email protected]> Kokoro: Kokoro <[email protected]> Kokoro: Ben Clayton <[email protected]>

ben-clayton · 2023-10-26T10:29:22Z

Unsure if @p works in tsdoc, you can check the generated documentation if you want to be sure (but it doesn't really matter that much). Also the mouseover-popup in vscode will render the documentation for you.

The TSDoc doesn't list @p in the tags. I'll followup with a change to clean these up - I'm sure I've used them in a bunch of other places too.

This removes a the need to create bunch of temporary JSON objects, reducing the amount of garbage collection we need to do. This change also changes the DataCache to be unbounded to a 4-element LRU cache, capping the amount of memory used.

github-actions · 2023-10-26T10:52:38Z

Previews, as seen when this build job started (34dc503):
Run tests | View tsdoc

With gpuweb/cts#3094 landed, the case cache now produces binary files, not JSON files. Change-Id: I7d297c14d28a5d8247a8e7cf72483188a7e5bd8e Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/158181 Commit-Queue: Austin Eng <[email protected]> Kokoro: Kokoro <[email protected]> Reviewed-by: Austin Eng <[email protected]> Auto-Submit: Ben Clayton <[email protected]>

ben-clayton force-pushed the binary-case-cache branch from 9cc0289 to d238b9f Compare October 25, 2023 19:53

ben-clayton changed the title ~~WIP: Binary serialization for the case cache~~ Replace JSON case cache serialization with binary files Oct 25, 2023

ben-clayton marked this pull request as ready for review October 25, 2023 20:21

ben-clayton requested review from zoddicus and austinEng October 25, 2023 20:21

ben-clayton force-pushed the binary-case-cache branch from d238b9f to 0936974 Compare October 25, 2023 20:33

zoddicus requested a review from kainino0x October 25, 2023 20:37

zoddicus reviewed Oct 25, 2023

View reviewed changes

austinEng approved these changes Oct 25, 2023

View reviewed changes

ben-clayton force-pushed the binary-case-cache branch from 0936974 to 0651deb Compare October 25, 2023 20:46

ben-clayton force-pushed the binary-case-cache branch from 0651deb to 8614162 Compare October 26, 2023 10:34

ben-clayton force-pushed the binary-case-cache branch from 8614162 to 34dc503 Compare October 26, 2023 10:40

ben-clayton merged commit 6e21caa into gpuweb:main Oct 26, 2023
2 checks passed

ben-clayton deleted the binary-case-cache branch October 26, 2023 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace JSON case cache serialization with binary files #3094

Replace JSON case cache serialization with binary files #3094

ben-clayton commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023

zoddicus left a comment

austinEng left a comment

austinEng Oct 25, 2023

ben-clayton Oct 25, 2023

ben-clayton commented Oct 25, 2023

github-actions bot commented Oct 25, 2023

austinEng commented Oct 25, 2023

kainino0x commented Oct 25, 2023

kainino0x commented Oct 25, 2023

ben-clayton commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

Replace JSON case cache serialization with binary files #3094

Replace JSON case cache serialization with binary files #3094

Conversation

ben-clayton commented Oct 25, 2023 • edited Loading

github-actions bot commented Oct 25, 2023

zoddicus left a comment

Choose a reason for hiding this comment

austinEng left a comment

Choose a reason for hiding this comment

austinEng Oct 25, 2023

Choose a reason for hiding this comment

ben-clayton Oct 25, 2023

Choose a reason for hiding this comment

ben-clayton commented Oct 25, 2023

github-actions bot commented Oct 25, 2023

austinEng commented Oct 25, 2023

kainino0x commented Oct 25, 2023

kainino0x commented Oct 25, 2023

ben-clayton commented Oct 26, 2023

github-actions bot commented Oct 26, 2023

ben-clayton commented Oct 25, 2023 •

edited

Loading