performance and other stuff from https://github.com/schteppe/p2.js/pull/366 recreated

amytimed commented 1 year ago

basically, i saw https://github.com/schteppe/p2.js/pull/366, and then i saw this, and then i thought "wouldnt it be really cool to have both in one, typescript maintained version + the perf benefits?" and then i did that

passes all tests

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
p2-es	❌ Failed (Inspect)			Oct 12, 2023 1:32am

isaac-mason commented 1 year ago

Thanks for the PR! I will review this soon 🙂

isaac-mason commented 11 months ago

Thanks again for the PR!

This PR has a bit in it, so I've separated out the change for deferring addition and removal of objects while stepping into a separate commit, this is a WIP on the next branch, see: https://github.com/pmndrs/p2-es/commit/eabee3fefd9f3c88869260b68e8d9bcd1f15c756 (I've made sure to credit you in the changeset! 🙂)

I made a few changes too:

Deferred changes are handled after every internal step
Deferred changes are replayed in order (e.g. if you added a body, removed it, then added it back, this would behave as expected)
Modified the existing duringStep tests to assert that changes are correctly deferred
Updated JSDoc explaining the behaviour difference when adding/removing while stepping

Also, I've created an issue to track this and other potential improvements: https://github.com/pmndrs/p2-es/issues/130

I'll look to get this into master and released soon!

Regarding the other performance changes caching length, it would be good to do some benchmarking to understand what the change in performance is before merging. I'm happy to help with this 🙂

As for what we should do with this PR, we can either close it and create a new PR based on next just adding the performance changes, or we can repurpose this one and do the same.

If you have time to do either of the above, feel free to, otherwise I can in the coming weeks!

LMK if you have any questions, and thanks again

amytimed commented 11 months ago

Regarding the other performance changes caching length, it would be good to do some benchmarking to understand what the change in performance is before merging.

it would definitely be helpful to do some benchmarking for that, but in general even if it isn't that much faster, it would be better practice to cache length instead of getting it every time

theres also other performance changes like using for instead of indexOf, which the original PR author claims increased performance, and which seems to be backed up by some articles I found: https://medium.com/@mrashes2/indexof-vs-for-loop-6a9f7bd5c646 though I have not tested this myself

pmndrs / p2-es

performance and other stuff from https://github.com/schteppe/p2.js/pull/366 recreated #118