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

Faster (and safer) equal implementation #685

Merged
merged 2 commits into from Apr 4, 2022
Merged

Faster (and safer) equal implementation #685

merged 2 commits into from Apr 4, 2022

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Mar 29, 2022

Hello! Love dnd-kit, it's working great in my application.

I'm dealing with quite a lot of data - benchmarking with 40,000 items in a sortable list. dnd-kit is mostly not a bottleneck, but I have noticed that, since I'm using uuids and quite a lot of items, the isEqual method takes up an appreciable amount of the performance profile, and the size of the strings generated can trigger a GC.

Hence: a non-join based isEqual implementation. I did bench this to make sure that it resolves the performance issues - seems to, about a 15x gain, not counting the reduced memory pressure.

joinEqual/short x 1,613,252 ops/sec ±0.66% (89 runs sampled)
joinEqual/equal x 1,808 ops/sec ±0.65% (90 runs sampled)
joinEqual/notEqual x 1,891 ops/sec ±0.59% (93 runs sampled)
joinEqual/differentLength x 1,879 ops/sec ±0.89% (90 runs sampled)
iterativeEqual/short x 23,996,960 ops/sec ±2.75% (87 runs sampled)
iterativeEqual/equal x 28,280 ops/sec ±2.18% (85 runs sampled)
iterativeEqual/notEqual x 24,164,244 ops/sec ±0.64% (91 runs sampled)
iterativeEqual/differentLength x 24,280,053 ops/sec ±0.52% (90 runs sampled)

Benchmark here

This also fixes a bug in dnd-kit, in which you could have different ID sequences that join to the same thing. For example, if you had: [a, bb, c], and the updated item ids were [ab, b, c] or any other combination of (abbc) then you'd get a false 'equal' through the join-based method.

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2022

🦋 Changeset detected

Latest commit: 1aa29e0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@dnd-kit/sortable Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@clauderic
Copy link
Owner

Hey @tmcw, thanks for your contribution! This looks great! Would you mind adding a changeset or giving me write access?

@tmcw
Copy link
Contributor Author

tmcw commented Apr 1, 2022

Yep! Done.

@clauderic clauderic merged commit 4b286e4 into clauderic:master Apr 4, 2022
@github-actions github-actions bot mentioned this pull request Apr 4, 2022
@tmcw
Copy link
Contributor Author

tmcw commented May 3, 2022

👋 hey - any chance that this might get released? Still seeing this in my profiles and very excited to remove that bottleneck

@clauderic
Copy link
Owner

Hey @tmcw, every merged commit gets deployed to npm under the next tag, so you can already upgrade to the packages of @dnd-kit tagged with next:

npm install @dnd-kit/core@next

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.

None yet

2 participants