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

BatchedMesh: Add setCustomSort function, hybrid radix sort implementation to examples #27213

Merged
merged 9 commits into from
Nov 21, 2023

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Nov 19, 2023

Related issue: #22376, #27202 (comment)

Description

For the sake of discussion here's a draft PR that shows the performance improvements using the suggested sort implementation from #27202 (comment). If you want to try it you can run the batched example page and toggle the "useRadix" option to see the difference. The sort time is logged in the console.

Performance

Overall it looks like the new sort is providing roughly a 10x improvement in sort time over the native sort information. The drawback of the sort is that it requires more memory to store a temporary array:

hybrid sort Array.sort
sort time ~0.65ms ~6.3ms
fps ~94fps ~74fps

There seem to be occasional dips in fps with the hybrid sort to ~89fps on my machine but I'm not sure why.

Overall it's a meaningful improvement in complex cases. I'm not sure what opinions of others are on whether this should be included in three, though.

cc @Mugen87 @mrdoob @sciecode

Copy link

github-actions bot commented Nov 19, 2023

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
668.3 kB (165.9 kB) 668.4 kB (165.9 kB) +105 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
449.4 kB (108.9 kB) 449.4 kB (108.9 kB) +0 B

@CodyJasonBennett
Copy link
Contributor

There seem to be occasional dips in fps with the hybrid sort to ~89fps on my machine but I'm not sure why.

I haven't delved much into the implementation yet, but my first thought is that maybe garbage collection is playing a role here.

@gkjohnson
Copy link
Collaborator Author

I haven't delved much into the implementation yet, but my first thought is that maybe garbage collection is playing a role here.

That was my first thought, too - I had missed that an array was getting created internally on every call when I thought was cached so that was likely it. Just pushed the fix.

@sciecode
Copy link
Contributor

sciecode commented Nov 19, 2023

I get similar sorting times, but slightly better overall performance ( ~40% )

hybrid sort Array.sort
sort ~0.69ms ~6.5ms
fps ~89fps ~62fps

I believe radix-sort can be a beneficial addition to three, but it's important to inform the users of the restrictions, as it is a less general function than Array.sort.

There are currently two important restrictions:

  • It expects values in range [0, UINT32_MAX]
  • array.length <= UINT32_MAX

The first restriction is not bad as it might seem, it does work on floating-point numbers if they are normalized to the expected range, as Garrett did in the example (see below). And the same thing can be done with signed 32-bit integers, like so:

// UINT32_MAX = 4_294_967_295
// INT32_MAX+1 = 2_147_483_648
const remap = (s32) => s32 + INT32_MAX + 1;

edit: positive floating point values should be bit-cast, instead of normalizd - otherwise truncated values produce incorrect ties

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 19, 2023

TBH, I would prefer to keep this outside of the core since BatchedMesh is already a quite complex component and we should not over-engineer it.

Could you add something like BatchedMesh.setCustomSort() so every developer can add a custom sort implementation if performance would be an issue?

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Nov 20, 2023

TBH, I would prefer to keep this outside of the core since BatchedMesh is already a quite complex component and we should not over-engineer it.

Could you add something like BatchedMesh.setCustomSort() so every developer can add a custom sort implementation if performance would be an issue?

It's a pretty significant improvement but admittedly likely only really relevant for particularly complex cases. How would you feel about something like a fastBatchedMeshSort and / or hybridRadixSort to the three.js examples along with a "setCustomSort" function so this code is available?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 20, 2023

So you mean moving src/objects/radixSort.js to the addons and make a new example that shows how to use it via setCustomSort()? That sounds good to me 👍 .

@gkjohnson gkjohnson marked this pull request as ready for review November 20, 2023 11:36
@gkjohnson
Copy link
Collaborator Author

I've adjusted the PR to move the radix sort function into a "SortUtils.js" example file, added a "customSort" function to BatchedMesh, and updated the example to include a "useCustomSort" function.

@gkjohnson gkjohnson changed the title BatchedMesh: Add hybrid radix sort to test performance BatchedMesh: Add setCustomSort, hybrid radix sort implementation to examples Nov 20, 2023
@gkjohnson gkjohnson changed the title BatchedMesh: Add setCustomSort, hybrid radix sort implementation to examples BatchedMesh: Add setCustomSort function, hybrid radix sort implementation to examples Nov 20, 2023
AdaRoseCannon pushed a commit to AdaRoseCannon/three.js that referenced this pull request Jan 15, 2024
…tion to examples (mrdoob#27213)

* Add radix sort to batched mesh

* Fix options generation

* Add custom sort function, separate radix sort function to examples

* Adjust custom sort application

* revert count and position in demo

* Update demo

* Add docs

* Comments
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.

4 participants