Skip to content

feat: Cast elements to correct type when creating vectors #1311

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

aleksanderkatan
Copy link
Contributor

No description provided.

@aleksanderkatan aleksanderkatan linked an issue May 28, 2025 that may be closed by this pull request
Aleksander Katan added 2 commits May 28, 2025 13:43
Copy link

github-actions bot commented May 28, 2025

pkg.pr.new

packages

pnpm i https://pkg.pr.new/software-mansion/TypeGPU/typegpu@1311
pnpm i https://pkg.pr.new/software-mansion/TypeGPU/typegpu@89b285cdad7336f1c28714a83789a5e5266d1229

benchmark
view benchmark

commit
view commit

@aleksanderkatan aleksanderkatan marked this pull request as ready for review May 28, 2025 12:11
@@ -834,13 +850,13 @@ describe('v3i', () => {
describe('v4f', () => {
describe('(v3f, number) constructor', () => {
it('works in JS', () => {
const red = d.vec3f(0.9, 0.2, 0.1);
const red = d.vec3f(0.125, 0.25, 0.375);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a little side effect of the float cast we do with the Float32Array
image

@aleksanderkatan
Copy link
Contributor Author

All those checks seem to have affected the performance a bit

image

@iwoplaza
Copy link
Collaborator

iwoplaza commented Jun 4, 2025

@aleksanderkatan Try the PR build, sometimes release builds are faster than local builds because of the transform that the bundler performs.

@aleksanderkatan
Copy link
Contributor Author

@iwoplaza if this is what you meant, then sadly there is no significant change

image

Copy link
Contributor

@mhawryluk mhawryluk left a comment

Choose a reason for hiding this comment

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

Well done!! 🚀

Copy link
Contributor

@reczkok reczkok left a comment

Choose a reason for hiding this comment

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

I'm really on the fence about this — I get that the consistency between JS and WGSL is nice, but is it really worth the performance hit (which, as we've established, is kind of important)?

@reczkok
Copy link
Contributor

reczkok commented Jun 5, 2025

I'm really on the fence about this — I get that the consistency between JS and WGSL is nice, but is it really worth the performance hit (which, as we've established, is kind of important)?

Maybe we could leave this in and figure out a way to provide users with a very fast mass vector constructor

@aleksanderkatan
Copy link
Contributor Author

aleksanderkatan commented Jun 5, 2025

I think we could modify the schema casts for scalar values, so that f32 and f16 are identity, i32 is Math.floor(n), u32 is Math.max(0, Math.floor(n)), and bool is left as is (all plus the undefined check). This would probably increase the performance and leave the most important part of the cast in place. What do you think?

Edit: Another benefit is that we'd avoid the floating point error in parsed wgsl.

@reczkok
Copy link
Contributor

reczkok commented Jun 5, 2025

I think we should keep the casting logic to stay compliant with WGSL's behavior. I’m actually curious how much of the performance hit comes from the casting itself versus the overhead of the function calls.

@aleksanderkatan
Copy link
Contributor Author

aleksanderkatan commented Jun 6, 2025

@reczkok seems like most of the overhead is in Math functions and array writes.

Before any changes:
image

When calling schema constructors, but all they do is return the value passed in:
image

When calling schema constructors, and casting all values properly:
image

Edit: the results aren't too consistent. Sometimes I get latest with 3500 ops/s, and once it returned 1500 ops/s

@aleksanderkatan
Copy link
Contributor Author

aleksanderkatan commented Jun 17, 2025

The benchmark results are now wildly inconsistent. For one test, I get 30 ops/s five times in a row, then I get 11 ops/s five times in a row...
What is consistent is that the PR version gets around 20% less ops/s on the biggest vector creation test than the latest.

I added the coercion to the vector element assignment (through [0] and through .x). I also removed the float value casts, and I simplified a bit the integer casts.

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.

Cast elements to correct type when creating vectors
4 participants