WebGL bindings: Fix shifts for >=2GB heaps #17832
Conversation
|
Testing this would require allocating over 2GB of memory, which is tricky on CI... we only have a few stress tests for that. Not sure if it's worth adding something for an issue like this. |
|
Added a disabled test. The test can only pass in Chrome Canary which has fixed some issues in this area recently. |
|
I still think this would be best to be fixed with #15433 , so it would work in general for all JS libraries. Although there was some hard pushback in the comments back then, which made me a bit disheartened to press on that PR, and other more urgent work came in the way. The approach in #15433 does still feel the best way forward to me though, and I'd love to take that in, since it would enable Unity to target 2GB+4GB+Wasm64 in a way that doesn't require teaching Unity JS game devs two different ways to manage pointers in JS land. Unfortunately I'll be on PTO after this week, so will be a short while until I can focus on #15433. One way might be to land this interim, and then later replace it with #15433 if we agree with that is the direction to go? LGTM on the code. |
| #else | ||
| return ptr >> shifts; | ||
| #endif | ||
| }, |
There was a problem hiding this comment.
In general I think this would be nice to do compile-time, #15433 can help with that. This way there wouldn't be a WebGL-specific function emitted to grow code size. But lgtm for now.
| def test_gl_glteximage(self): | ||
| self.btest('gl_teximage.c', '1', args=['-lGL', '-lSDL']) | ||
|
|
||
| @unittest.skip("can only work in chrome canary atm (Sep 2022)") |
There was a problem hiding this comment.
We use @disabled normally, which I think is just a synonym
|
I see @juj, thanks. Sounds good to me to wait for #15433 - this PR is not urgent, as testing this needs to wait on a Chrome update anyhow, so I wasn't planning to land it immediately. Let's wait for your PR, when you have time for it. (Please don't be discouraged by some of the comments in #15433, I think that PR is the right way forward, and recent work has been going that way anyhow. I am sure we can convince everyone of that, as necessary.) |
|
This has been fixed meanwhile - these GL methods use |
Fixes #17539