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

Array lists with set capacities in SimpleBatcher #2445

Closed
luciopaiva opened this issue Jul 12, 2023 · 4 comments
Closed

Array lists with set capacities in SimpleBatcher #2445

luciopaiva opened this issue Jul 12, 2023 · 4 comments
Labels
type: enhancement A general enhancement
Milestone

Comments

@luciopaiva
Copy link
Contributor

Feature Request

This is not exactly a feature request, but a potential enhancement request. While working on #2444, I noticed this array list with a set capacity:

List<RedisCommand<Object, Object, Object>> batch = new ArrayList<>(Math.max(batchSize, 10));

This logic actually appears three times inside SimpleBatcher. My questions are:

  • is there any reason for setting the capacity of these array lists? (It's unlikely that it is bringing any noticeable performance enhancements)
  • any specific reason for the max(..., 10)?

I'm asking this because in the code I'm working on, I thought about setting @BatchSize() to a huge value like Integer.MAX_VALUE in order to operate solely on manual flushes via flush() calls. The problem is that those lists are being preallocated according to the value in the annotation, so one quickly runs into an OOM if @BatchSize() is too big (that's how I found that code).

My enhancement proposal is to stop setting the capacity on those lists. In case my suggestion is welcome, I would also propose we further improve that code by not even having to allocate new lists inside prepareForceFlush() and prepareDefaultFlush(), but instead relying solely on the one created inside flush() (and thus avoid an unnecessary extra copy as well).

Thoughts? I'd be glad to create a PR for it, but I wanted to check here first if you think it makes sense.

Thanks!

@mp911de
Copy link
Collaborator

mp911de commented Jul 13, 2023

Preallocation makes some sense to avoid collection resizing. Indeed, the preallocation calculation is wrong, it should rather be Math.min(batchSize, queue.size()).

@mp911de mp911de added the type: enhancement A general enhancement label Jul 13, 2023
mp911de added a commit that referenced this issue Jul 17, 2023
Refine list preallocation size.
mp911de added a commit that referenced this issue Jul 17, 2023
Refine list preallocation size.
@mp911de
Copy link
Collaborator

mp911de commented Jul 17, 2023

That's fixed now.

@mp911de mp911de closed this as completed Jul 17, 2023
@mp911de mp911de added this to the 6.2.5.RELEASE milestone Jul 17, 2023
mp911de added a commit that referenced this issue Jul 17, 2023
Refine list preallocation size.
mp911de added a commit that referenced this issue Jul 17, 2023
Refine list preallocation size.
@luciopaiva
Copy link
Contributor Author

LGTM. WTG extracting a method there.

Future improvements:

  • pass the array list created in flush() to prepareForceFlush() and prepareDefaultFlush() and thus avoid an extra copy
  • merge prepareForceFlush() and prepareDefaultFlush() into a single method to simplify the logic

@mp911de
Copy link
Collaborator

mp911de commented Jul 17, 2023

Feel free to submit a pull request. Happy to merge improvements if they make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants