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

Consider fastutil instead of gnu trove #50

Open
benmccann opened this issue Aug 17, 2017 · 3 comments
Open

Consider fastutil instead of gnu trove #50

benmccann opened this issue Aug 17, 2017 · 3 comments

Comments

@benmccann
Copy link
Contributor

Trove is licensed under the LGPL, which isn't compatible with the Apache 2 license that Morpheus uses. Might not be a problem for all users, but some may wish to exclude Trove from their projects due to the license.

Trove has been around for a while, but it isn't as actively developed anymore. FastUtil is much more recent, and actively developed.

Fastutil also performs much better according to http://java-performance.info/hashmap-overview-jdk-fastutil-goldman-sachs-hppc-koloboke-trove-january-2015/

@benmccann
Copy link
Contributor Author

There's a branch which shows roughly the work required below. The code compiles, but five tests fail, so it would need to be fixed before it could be committed.

https://github.com/benmccann/morpheus-core/commits/fastutil

@Zavster
Copy link
Contributor

Zavster commented Aug 22, 2017

Thanks for doing this Ben. I have made some changes to fix the tests, and also ensured that all calls to fastutil use the non-boxed and non-deprecated methods. I pushed the changes to my fork on a branch called fastutil_2 located below. Try fetch that branch and confirm all the tests pass for you, and if so, raise a PR and I'll merge it. Thanks again!

https://github.com/Zavster/morpheus-core/tree/fastutil_2

Cheers,
Zav

@benmccann
Copy link
Contributor Author

benmccann commented Aug 22, 2017

Thanks! Probably easier for you to create the PR since you already have it pushed to GitHub.

I see you have this in a few places:

final int capacity = limit < it.unimi.dsi.fastutil.Arrays.MAX_ARRAY_SIZE ? limit : 1000;

Maybe better to change it to something like the below?

if (limit > it.unimi.dsi.fastutil.Arrays.MAX_ARRAY_SIZE) {
    throw new IllegalArgumentException("Specified limit " + limit + " is greater than max limit " + it.unimi.dsi.fastutil.Arrays.MAX_ARRAY_SIZE);
}

Or at least log a warning or something so that it doesn't just magically do something different than the user is expecting

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

No branches or pull requests

2 participants