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

Add KTypeArrayList removeAt and rename remove to removeElement. #247

Merged
merged 2 commits into from
May 24, 2024

Conversation

bruno-roustant
Copy link
Collaborator

Optimize removal methods to only dereference removed elements when the list is generic.
Add test and make several existing tests run on primitive types.

@dweiss dweiss added this to the 0.10.0 milestone May 24, 2024
final int deleted = elementsCount - to;
this.elementsCount = to;
/* #if ($TemplateOptions.KTypeGeneric) */
Arrays.fill(buffer, elementsCount, elementsCount + deleted, Intrinsics.empty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's going to be faster this way? Those null assignments should be within the same cache line, so super-fast already? And previously it'd zero the removed slot, now it doesn't. Don't know if it makes a difference in practice.

Copy link
Collaborator Author

@bruno-roustant bruno-roustant May 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you asked the question, I did a benchmark running removeAll on lists of size 10, 100, 1_000, 10_000, and with four categories of number of duplicate elements to make the removal longer.

  • Surprisingly (to me) I see no perf difference for primitive IntArrayList with and without zeroing removed slots.
  • For ObjectArrayList, the new version with Arrays.fill is 20% faster for large lists.

So I would tend to keep this new code, unless you see an interest in zeroing removed slots for primitives. BTW the KTypeSlack implementation of pop was already not zeroing removed slots, and that suggested me to do the same for list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for running the benchmark. LGTM.

Optimize removal methods to only dereference removed elements when the list is generic.
@bruno-roustant bruno-roustant merged commit 3ca9dc2 into carrotsearch:master May 24, 2024
2 checks passed
@bruno-roustant bruno-roustant deleted the container_remove branch May 24, 2024 13:54
This pull request was closed.
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