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

OAK-11112 : replaced Maps.newHashMapWithExpectedSize() with new HashM… #1713

Merged
merged 3 commits into from
Sep 17, 2024

Conversation

rishabhdaim
Copy link
Contributor

…ap<>(initialCapacity)

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

I believe for the size calculation we should java common utility method, or even implement newHash* directly in CollectionUtils. @nfsantos ?

@@ -34,6 +36,8 @@
*/
public class CollectionUtils {

private static final int MAX_CAPACITY = 1 << 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

@rishabhdaim rishabhdaim Sep 17, 2024

Choose a reason for hiding this comment

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

This is the maximum capacity of hashmap. Any value above this would be reverted to this value.

Also, we might hit negative values if we don't specify an upper limit.

See test below:

@Test public void ensureCapacityWithMaxValue() { int capacity = CollectionUtils.ensureCapacity(1908874369); }

it returns -2147483648, which is wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe also add a comment with a short explanation?

Copy link
Contributor

@reschke reschke left a comment

Choose a reason for hiding this comment

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

Could you please separate the changes for CollectionUtils from the cases where we use them?

@rishabhdaim
Copy link
Contributor Author

@reschke Created #1722 for ensureCapacity method changes.

Copy link
Contributor

@mbaedke mbaedke left a comment

Choose a reason for hiding this comment

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

If we don't care that the maps will resize, it looks fine. Otherwise we might want to increase the initial capacity.

@rishabhdaim
Copy link
Contributor Author

@mbaedke we are using CollectionUtils.newHashMap API which creates a hashmap with the expected size to accommodate the expected number of elements without resizing.

@rishabhdaim rishabhdaim merged commit 874052d into trunk Sep 17, 2024
1 of 2 checks passed
@rishabhdaim rishabhdaim deleted the OAK-11112 branch September 17, 2024 12:42
@mbaedke
Copy link
Contributor

mbaedke commented Sep 17, 2024

@mbaedke we are using CollectionUtils.newHashMap API which creates a hashmap with the expected size to accommodate the expected number of elements without resizing.

Yes, of course. I was confused, sorry for the noise.

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.

3 participants