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

Upgrade ABV offset args from u32->u64. #3586

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

kdashg
Copy link
Contributor

@kdashg kdashg commented Sep 7, 2023

Not sizes right now because we don't expect to support >4G sizes for uploads.

This change better supports large WebAssembly heaps. Related to crbug.com/1476859 ;
conformance test for all browsers forthcoming.

Not sizes right now because we don't expect to support >4G sizes for
uploads.
@CLAassistant
Copy link

CLAassistant commented Sep 7, 2023

CLA assistant check
All committers have signed the CLA.

@kdashg
Copy link
Contributor Author

kdashg commented Sep 7, 2023

Instead of adding a typedef or using one of the (signed) existing ones, I opted to just use "unsigned long long" directly. These aren't types we're passing down into drivers anyways.

@lexaknyazev
Copy link
Member

Similar update should probably be done to offset parameters in:

  • WEBGL_multi_draw
  • WEBGL_multi_draw_instanced_base_vertex_base_instance
  • WEBGL_shader_pixel_local_storage

Copy link
Member

@kenrussell kenrussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes; they look perfect. It's unfortunate there's no typedef for "unsigned long long" in the WebGL 1.0 IDL.

Would you like to update the extensions Alexey pointed out in this patch?

I updated the PR comment to give a bit more background; hope it's OK, or feel free to edit further. Would like that to show up in the commit message.

@kenrussell
Copy link
Member

Note @juj that this spec change is incoming - the intent is to support Wasm64 better. Please tell us if you have any concerns.

@lexaknyazev
Copy link
Member

Instead of adding a typedef or using one of the (signed) existing ones, I opted to just use "unsigned long long" directly.

Looks like WebGL 2.0, Section 3.1 Types defines it:

typedef unsigned long long GLuint64;

@kenrussell
Copy link
Member

Instead of adding a typedef or using one of the (signed) existing ones, I opted to just use "unsigned long long" directly.

Looks like WebGL 2.0, Section 3.1 Types defines it:

typedef unsigned long long GLuint64;

Thanks, didn't check the WebGL 2.0 spec. @kdashg is it feasible to use that? Would you update the extension IDLs as well?

@kenrussell
Copy link
Member

Instead of adding a typedef or using one of the (signed) existing ones, I opted to just use "unsigned long long" directly.

Looks like WebGL 2.0, Section 3.1 Types defines it:

typedef unsigned long long GLuint64;

Thanks, didn't check the WebGL 2.0 spec. @kdashg is it feasible to use that? Would you update the extension IDLs as well?

Thinking about it more it may not be a good idea to use a WebGL 2.0-specific typedef in the extensions' IDL. The use of unsigned long long is fine by me.

@lexaknyazev
Copy link
Member

Thinking about it more it may not be a good idea to use a WebGL 2.0-specific typedef in the extensions' IDL.

Agreed wrt WEBGL_multi_draw. This PR targets the core 2.0 spec though.

@kenrussell
Copy link
Member

Talked with @kdashg ; going to merge this and follow up with a PR that updates the extensions. Thanks @kdashg and @lexaknyazev .

@kenrussell kenrussell merged commit 419b3c6 into KhronosGroup:main Sep 8, 2023
1 check passed
kenrussell added a commit to kenrussell/WebGL that referenced this pull request Sep 8, 2023
Updates the ArrayBufferView offset arguments in the following extensions:

  WEBGL_multi_draw
  WEBGL_multi_draw_instanced_base_vertex_base_instance
  WEBGL_shader_pixel_local_storage

to better support large WebAssembly heaps.

Follow-on to KhronosGroup#3586.
kenrussell added a commit that referenced this pull request Sep 8, 2023
Updates the ArrayBufferView offset arguments in the following extensions:

  WEBGL_multi_draw
  WEBGL_multi_draw_instanced_base_vertex_base_instance
  WEBGL_shader_pixel_local_storage

to better support large WebAssembly heaps.

Follow-on to #3586.
@juj
Copy link
Contributor

juj commented Sep 11, 2023

I understand that this change should not visibly affect old Wasm code that uses only <= 4GB heaps? Since unsigned long long is still always a JS Number(?)

Though in Wasm64, all 64-bit integers flow from Wasm into JavaScript side as BigInts. With this API, the JS WebGL shims still need to cast all BigInts into Number()s. I wonder if it would make sense to support passing in BigInts into these APIs directly, without needing to do a BigInt -> Number() cast? Maybe WebIDL could evolve a uint64AsNumberOrAsBigInt kind of type annotation to support this?

Not that any Wasm64 page today would be able to use more than 53-bit long offsets, since Wasm64 still does not have virtual memory support (i.e. user cannot do a virtual allocation to an arbitrary high part of the address space). But I wonder if directly allowing taking in both Numbers and BigInts that would be expected to be of u64 size would be able to give a size win and a developer ergonomy win? And in case if Wasm one day would get virtual allocation mechanisms (I presume one day it will have to), then there would be no need to do yet another IDL update.

(similar question is interesting for WebGPU)

CC @aardappel @dschuff

@dschuff
Copy link

dschuff commented Sep 11, 2023

/cc @sbc100

@dschuff
Copy link

dschuff commented Sep 11, 2023

So I don't know if I understand this change yet, but:
Even in wasm32, unsigned long long is a 64-bit integer (long types, like pointer types, are 32-bit in wasm32 and 64-bit in wasm64). If the bigint feature is not available, then it's not possible to import or export a function to JS that takes or returns an i64 value. With the bigint feature, then such functions take or return JS bigints. (This is technically independent of wasm64, although I don't expect there will be any implementations of wasm64 that don't also support bigint).
Emscripten distinguishes pointer from true i64 types in its function signature system and, when used in wasm64 mode, will automatically convert bigints that are marked as pointers at the boundary (the idea being that Numbers are much easier to deal with in JS, and 53 bits ought to be large enough to address any practical memory). I don't think this is really documented yet, @sbc100 could say more.

But I would think that when designing a JS API that explicitly models pointers or similar address indexes, that it would be easier to use with Numbers rather than bigints.

@kenrussell
Copy link
Member

Changing these offset arguments to take BigInt or a union allowing BigInt would significantly reduce the performance of these entry points; the unboxing process, or even handling the union, would require runtime calls into the JavaScript engine, slowing down the bindings. In Chromium this would eliminate the possibility of using fast API calls.

As written, these entry points would support placement of the source data anywhere in the first 2^53 bytes of the Wasm heap. This seems to me to be enough flexibility, but please correct me if I'm wrong.

@juj
Copy link
Contributor

juj commented Sep 11, 2023

Emscripten distinguishes pointer from true i64 types in its function signature system and, when used in wasm64 mode, will automatically convert bigints that are marked as pointers at the boundary (the idea being that Numbers are much easier to deal with in JS, and 53 bits ought to be large enough to address any practical memory)

Yes, for an Emscripten end user, the mechanism looks like it is automatic, if you decide to invoke the automatic conversion mechanism. I am not contemplating as an Emscripten end user, but the fact that the conversion does exist in the generated JS code, and if there would be a way to annotate into the IDL a door to get those int64s directly into the browser APIs as the original int64s.

The thought I had here is that while we are right now updating the IDL, if it would make sense to update directly to allow a max 64-bit sized BigInt. That would avoid the need to revisit this again if Wasm64 virtual memory techniques come up in the future, if it was all the same to do the "right thing" today.

Changing these offset arguments to take BigInt or a union allowing BigInt would significantly reduce the performance of these entry points; the unboxing process, or even handling the union, would require runtime calls into the JavaScript engine, slowing down the bindings. In Chromium this would eliminate the possibility of using fast API calls.

A reasonable JIT could identify the BigInts flowing out from Wasm to have fixed 64-bit shape, and when such numbers flow into a web API call, see that it is a 64-bit integer and take the appropriate entry point. That would avoid int64 -> double -> int64 conversions in the flow of Wasm->JS->Web API, and allow removing current validations of whether a double represents an integer value or not.

Anyways, it reads like the idea sounds alien, and best to revisit the IDL again if wasm64 will get virtual memory addressing in the future.

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.

6 participants