Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 7, 2024

This was caused by #22557, which was checking the LibraryWebGPU object for __sig members when the sigs are stored in a separate file and therefore live on the global LibraryManager.library object.

Fixes: #22682

@sbc100 sbc100 requested a review from juj October 7, 2024 19:37
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 7, 2024

Our webgpu testing is somewhat limited so I'm not sure how to go about testing this. I'll give it a go though.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 7, 2024

It looks like adding a test that included a call to wgpuQueueWriteBuffer should reproduce this. Would it be easy to add a call to this to one of these existing webgpu browser tests @kainino0x ?

@juj
Copy link
Collaborator

juj commented Oct 7, 2024

Ah right of course.. the issue wasn't about order, but about the pre vs post addition name.

LGTM.

Btw what makes me a bit sad about the autogeneration aspect of library_sigs.js is that it increases to that "rift" of two tiers of JS library files: i.e. Emscripten authors have a different "power" or capabilities to JS library files that Emscripten users don't. This results in Emscripten users sometimes not having as easy time to monkey-see/-do copy stuff into their JS libraries. Ideally the JS libraries we have in Emscripten repository should be written using the same machinery as what end users have at their disposal. (dogfooding, and ensuring that APIs are complete, etc.)

Though I definitely get the usefulness of autogenerating the sigs from the headers.

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 7, 2024

Ah right of course.. the issue wasn't about order, but about the pre vs post addition name.

LGTM.

Btw what makes me a bit sad about the autogeneration aspect of library_sigs.js is that it increases to that "rift" of two tiers of JS library files: i.e. Emscripten authors have a different "power" or capabilities to JS library files that Emscripten users don't. This results in Emscripten users sometimes not having as easy time to monkey-see/-do copy stuff into their JS libraries. Ideally the JS libraries we have in Emscripten repository should be written using the same machinery as what end users have at their disposal. (dogfooding, and ensuring that APIs are complete, etc.)

Though I definitely get the usefulness of autogenerating the sigs from the headers.

Agreed. I think we should be able to make tools/maint/gen_sig_info.py usable for outside libraries perhaps? Although its a little more tricky than gen_struct_info.py since the input C/C++ headers might need more complex setups (e.g. include paths and macros), in order to compile.

@juj
Copy link
Collaborator

juj commented Oct 7, 2024

It looks like adding a test that included a call to wgpuQueueWriteBuffer should reproduce this. Would it be easy to add a call to this to one of these existing webgpu browser tests @kainino0x ?

In wasm_webgpu I concocted this kind of a test for wgpu_queue_write_buffer(). Even though the bindings API is completely different, the same API call flow should work - maybe that can be used as inspiration.

@sbc100 sbc100 force-pushed the fix_webgpu_bigint branch from 8e8d237 to 7f1606f Compare October 7, 2024 21:12
@ypujante
Copy link
Contributor

ypujante commented Oct 8, 2024

@sbc100 Thanks for fixing the problem

@juj Thanks for the thorough investigation

@kainino0x
Copy link
Collaborator

It looks like adding a test that included a call to wgpuQueueWriteBuffer should reproduce this. Would it be easy to add a call to this to one of these existing webgpu browser tests @kainino0x ?

If we only need to call it - and not check the buffer contents afterward - we could easily write to a buffer somewhere, e.g. here:
https://github.com/emscripten-core/emscripten/blob/main/test/webgpu_basic_rendering.cpp#L225-L226
use writeBuffer to put some data in the buffer, before overwriting it afterward.

@sbc100 sbc100 force-pushed the fix_webgpu_bigint branch from 7f1606f to 35f1c56 Compare October 8, 2024 16:37
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 8, 2024

It looks like adding a test that included a call to wgpuQueueWriteBuffer should reproduce this. Would it be easy to add a call to this to one of these existing webgpu browser tests @kainino0x ?

If we only need to call it - and not check the buffer contents afterward - we could easily write to a buffer somewhere, e.g. here: https://github.com/emscripten-core/emscripten/blob/main/test/webgpu_basic_rendering.cpp#L225-L226 use writeBuffer to put some data in the buffer, before overwriting it afterward.

Thanks, that worked. I verified locally that this test now fails without this change.

BTW, we don't run that test as part of CI because we don't have access to webgpu on our circleci test bots. Any ideas on how we could get this test running in headless mode on our linux CI bots?

This was caused by emscripten-core#22557, which was checking the `LibraryWebGPU` object
for `__sig` members when the sigs are stored in a separate file and
therefore live on the global `LibraryManager.library` object.

Fixes: emscripten-core#22682
@sbc100 sbc100 force-pushed the fix_webgpu_bigint branch from 35f1c56 to 65fe7c2 Compare October 8, 2024 16:39
@sbc100 sbc100 merged commit 9b8ed7e into emscripten-core:main Oct 8, 2024
1 check passed
@sbc100 sbc100 deleted the fix_webgpu_bigint branch October 8, 2024 16:40
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 8, 2024

Oops, I should probably have waited for an LGTM on that new test code. Can you take a look @kainino0x ?

@kainino0x
Copy link
Collaborator

Test LGTM!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebGPU Regression?

4 participants