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

Remove vek dependency. #34

Merged
merged 1 commit into from
Dec 31, 2024

Conversation

TannerRogalsky
Copy link
Contributor

Up front: I won't feel slighted if you decline this PR. I mostly did this for my own use but I figured I'd push upstream just in case.

I initially wanted just to remove the serde dependency from my tree but when I got in here and started looking at the code I figured I could trim my tree even more since the usage of vek in this project is restricted to the rasterizer implementations.

Changelog:

  • Replace vek with float arrays
  • Remove features associated with vek
  • Simplify feature variants (should be more difficult to create a feature combination that doesn't compile)

Was pleasantly surprised to see that there was a performance improvement. Auto-vectorization never ceases to amaze.

teapot/[1, 1]           time:   [3.1923 ms 3.2334 ms 3.2857 ms]
                        change: [-23.022% -21.423% -19.842%] (p = 0.00 < 0.05)
                        Performance has improved.

                        change: [-25.687% -23.491% -21.631%] (p = 0.00 < 0.05)
                        Performance has improved.
  2 (6.25%) high mild
  1 (3.12%) high severe

teapot/[640, 480]       time:   [5.1802 ms 5.1946 ms 5.2099 ms]
                        change: [-22.849% -20.976% -19.161%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 32 measurements (9.38%)
  1 (3.12%) high mild
  2 (6.25%) high severe

teapot/[1024, 800]      time:   [6.7946 ms 6.9040 ms 7.0218 ms]
                        change: [-28.973% -26.394% -23.945%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 32 measurements (9.38%)
  3 (9.38%) high mild

Benchmarking teapot/[2048, 1600]: Warming up for 1.0000 s
Warning: Unable to complete 32 samples in 5.0s. You may wish to increase target time to 8.8s, enable flat sampling, or reduce sample count to 10.
teapot/[2048, 1600]     time:   [17.232 ms 17.499 ms 17.765 ms]
                        change: [-24.200% -21.514% -18.662%] (p = 0.00 < 0.05)
                        Performance has improved.

teapot/[4096, 3200]     time:   [55.785 ms 56.339 ms 56.937 ms]
                        change: [-29.571% -28.268% -26.929%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 32 measurements (3.12%)
  1 (3.12%) high mild

@zesterer
Copy link
Owner

Happy to accept this change, although most work is being done on the refactor branch where, IIRC, this has already been done.

@TannerRogalsky
Copy link
Contributor Author

Maybe that branch hasn't been pushed as it's pretty behind on github.

@zesterer
Copy link
Owner

My bad, I'd forgotten the branch had been merged.

Happy to merge this, although I can't deny the increase in boilerplate does make me a little sad. Was there any particular reason that you were looking to simplify your dependency tree?

@TannerRogalsky
Copy link
Contributor Author

Particularly the inclusion of serde and it's proc macro has a disproportionate impact on my compile times. I'm also compiling this to a wasm target so am a little more conscientious of additional code payload than I would normally be for a native binary.

@zesterer
Copy link
Owner

zesterer commented Dec 20, 2024

Have you tried simply disabling default features for vek? I imagine that cuts all of that out.

Code-wise, vek is pretty much entirely generic code so it's pay-as-you-go in terms of extra binary size (the compiler will flatten the abstractions down well anyway). That aside, any linker worth its salt will remove unreferenced symbols if told to do so.

@TannerRogalsky
Copy link
Contributor Author

TannerRogalsky commented Dec 20, 2024 via email

@zesterer
Copy link
Owner

In that case, would configuring vek to have default features disabled be satisfactory for you?

@TannerRogalsky
Copy link
Contributor Author

TannerRogalsky commented Dec 31, 2024

At this point, I wouldn't go back to using vek in my fork of this project. Barring results from more robust benchmarks, the performance characteristics for both compile time and runtime are steering me away from having vek in my dependency tree at all.

That said, I'm not going to maintain my fork except for myself so I certainly understand your reluctance to reinvent the wheel for these math operations. If serde had been easier to remove from my dependency tree I probably would have not cared about this in the first place.

@zesterer
Copy link
Owner

That's fair enough. I do want to try to experiment with a few things: I think vek::Lerp::lerp does a range clamp, so that might explain some of the performance difference here. I'm happy to merge this though.

@zesterer zesterer merged commit e8f7aee into zesterer:master Dec 31, 2024
1 check passed
@TannerRogalsky TannerRogalsky deleted the dependency_cleanup branch December 31, 2024 12:26
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.

2 participants