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

Optimize SmallPersistentVector.removeAll #134

Closed

Conversation

mcpiroman
Copy link
Contributor

Buffer elements only after the first one is removed, instead of full copy.

@qurbonzoda
Copy link
Contributor

Hi @mcpiroman
Thanks for your effort.

Could you please clarify what the change optimizes? And how do they impact performance or memory footprint?

@mcpiroman
Copy link
Contributor Author

Hi,
currently if an element to be removed is found, a N-buffer is allocated, likely filled with zeros and filled with the master buffer. This theoretically gives N*2 iterations.

With this change:

  • the buffer is allocated with the size of the remaining part (after the first element to be removed), statistically this is N/2 for when single element is removed and even better when more.
  • the buffer is not a copy of master, only zeroed (arrayOfNulls instead of copyOf). Currently that copy in the part-after-first-removed is unnecessary and overridden.

This should result in smaller allocation and less iterations.
It is also possible to further reduce the size by delaying the allocation to the first retained after the first removed element, to help in case multiple elements are removed in a row. This would make the code slightly more complicated I suppose.

Now, I have run the relevant tests but I haven't run benchmarks as I don't have enough CPU and personal time atm.
Would it be maybe possible to integrate benchmarks in GH workflow (example)?

@ilya-g
Copy link
Member

ilya-g commented Dec 12, 2022

currently if an element to be removed is found, N-buffer is allocated, likely filled with zeros and filled with the master buffer

The new buffer is allocated with the copyOf function, which on JVM may avoid zeroing of a newly allocated array.

So a benchmark is indeed required to confirm the performance benefit of this change.

@qurbonzoda
Copy link
Contributor

Moved the review here: #164

@qurbonzoda qurbonzoda closed this Dec 21, 2023
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

3 participants