Skip to content

ImmutableKit.map with pre-size #2841

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.

Sign up for GitHub

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

Jump to bottom
Merged
bbakerman merged 1 commit into from
Jun 8, 2022
Merged

ImmutableKit.map with pre-size #2841

bbakerman merged 1 commit into from
Jun 8, 2022

Conversation

@dfa1
Copy link
Contributor

@dfa1 dfa1 commented May 30, 2022 •
edited
Loading

@bbakerman @andimarek a breaking change in ImmutableKit that provides a nice "free lunch":
by switching the signature from Iterable to Collection it is possible to pre-size the builder to avoid any extra allocation.

The class is is marked as @Internal so maybe it not a problem at all :)

List size = 10000

Benchmark                                          Mode  Cnt     Score     Error  Units
ListBenchmark.benchmarkArrayList                  thrpt   10  1879.357 ± 185.666  ops/s
ListBenchmark.benchmarkImmutableCollectorBuilder  thrpt   10  1879.835 ±  16.009  ops/s
ListBenchmark.benchmarkImmutableListBuilder       thrpt   10  1916.382 ±  13.232  ops/s
ListBenchmark.benchmarkImmutableListBuilder2      thrpt   10  1996.370 ±  15.618  ops/s <- new
ListBenchmark.benchmarkListStream                 thrpt   10  1833.081 ±  19.807  ops/s

List size = 10

ListBenchmark.benchmarkArrayList                  thrpt   10  1932469.875 ± 250182.583  ops/s
ListBenchmark.benchmarkImmutableCollectorBuilder  thrpt   10  1765995.264 ±   7710.397  ops/s
ListBenchmark.benchmarkImmutableListBuilder       thrpt   10  1992740.542 ±  10385.303  ops/s
ListBenchmark.benchmarkImmutableListBuilder2      thrpt   10  2104911.110 ±   5790.337  ops/s <- new
ListBenchmark.benchmarkListStream                 thrpt   10  1826454.508 ±  10735.506  ops/s

jord1e
jord1e reviewed May 31, 2022
View reviewed changes
@dfa1 dfa1 force-pushed the performance-immutablekit-map branch from 60189be to ed1bb6d Compare May 31, 2022 16:05
@bbakerman
Copy link
Member

bbakerman commented Jun 8, 2022 

List size = 10000

ListBenchmark.benchmarkImmutableListBuilder       thrpt   10  1916.382 ±  13.232  ops/s
ListBenchmark.benchmarkImmutableListBuilder2      thrpt   10  1996.370 ±  15.618  ops/s <- new

List size = 10

ListBenchmark.benchmarkImmutableListBuilder       thrpt   10  1992740.542 ±  10385.303  ops/s
ListBenchmark.benchmarkImmutableListBuilder2      thrpt   10  2104911.110 ±   5790.337  ops/s <- new

if I read this right - then for large lists its better but for small lists its twice as bad??

The guava code has

static final int DEFAULT_INITIAL_CAPACITY = 4;

Now this is interesting - which should we optimise for? And does it matter in practice?

I am happy to break the contact (Iterable -> Collection) but I am unsure on what end we optimise for

@dfa1
Copy link
Contributor Author

dfa1 commented Jun 8, 2022

actually the thrpt it is better for both sizes: only the error is twice as big for small lists 10385 vs 5790. Not sure if that really matters 🤷🏻‍♂️

@bbakerman
Copy link
Member

bbakerman commented Jun 8, 2022

if I read this right

Ahh yup - I was NOT reading it right

@bbakerman bbakerman added this to the 19.0 milestone Jun 8, 2022
@bbakerman bbakerman merged commit 5cac2e3 into graphql-java:master Jun 8, 2022
@bbakerman
Copy link
Member

bbakerman commented Jun 8, 2022

Thanks

@dfa1 dfa1 deleted the performance-immutablekit-map branch June 8, 2022 16:39
@dfa1
Copy link
Contributor Author

dfa1 commented Jun 8, 2022

I know it is not much, just a 4-6% thrpt increase in these 2 benchmarks.

@luiznaac luiznaac mentioned this pull request Aug 2, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jord1e jord1e jord1e left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

19.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dfa1 @bbakerman @jord1e