Skip to content

Optimize WeakConcurrentCollection removeAll and retainAll lookup complexity - #329

Closed
Smorki wants to merge 5 commits into
CraftCanvasMC:mainfrom
Smorki:perf/optimize-weak-concurrent-collection
Closed

Optimize WeakConcurrentCollection removeAll and retainAll lookup complexity#329
Smorki wants to merge 5 commits into
CraftCanvasMC:mainfrom
Smorki:perf/optimize-weak-concurrent-collection

Conversation

@Smorki

@Smorki Smorki commented Aug 23, 2026

Copy link
Copy Markdown

In WeakConcurrentCollection#removeAll and #retainAll, calling c.contains(value) inside the loop gets slow when c is a List.

This PR wraps c into a HashSet prior to iteration if it's not already a Set to make lookups O(1). It also handles empty collections cleanly to stay compliant with standard Java Collection behavior.

@BaconCat1

Copy link
Copy Markdown
Contributor

Why did you remove Objects/NonNull imports while retaining their uses? Does not compile.

@Smorki

Smorki commented Aug 23, 2026

Copy link
Copy Markdown
Author

oh iy was accidentally removed during the web conflict resolution. Just pushed a fix to restore the missing imports

@Smorki

Smorki commented Aug 23, 2026

Copy link
Copy Markdown
Author

🥀

@BaconCat1

Copy link
Copy Markdown
Contributor

Even after the compile issue is fixed, I don't think this is a good idea as written.
What actual Canvas workload is this optimizing? There aren't any Canvas call sites for WeakConcurrentCollection.removeAll or retainAll, so right now this adds complexity to a path that isn't even used.
It also isn't behaviorally equivalent. IdentityHashMap.values() is a non-Set collection with identity-based contains, and copying it into a HashSet changes the result for distinct but equal objects. That flips the behavior of both removeAll and retainAll.
This isn't always a performance win either. An empty or tiny receiver with a huge list still has to allocate and fill the entire HashSet, while repeated CopyOnWriteArrayList removals still dominate the worst case. Set doesn't guarantee constant-time lookups either, so the type check isn't a reliable way to choose this.
Unless there is a real call site and benchmark showing this helps, I don't think we should merge it. If there is, optimize that known path and keep collection.contains as the generic fallback.

@Smorki

Smorki commented Aug 23, 2026

Copy link
Copy Markdown
Author

Fair points, especially regarding the behavioral difference with identity-based collections and the lack of current call sites within Canvas I see now that adding this complexity might be premature without a concrete use case or benchmark.
illl close this for now to avoid adding unnecessary overhead where its not needed. Thanks for the detailed review!

@Smorki Smorki closed this Aug 23, 2026
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