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

arrow-core: speedup 'Arb.string()' related slow tests #3005

Merged
merged 9 commits into from
Mar 30, 2023

Conversation

abendt
Copy link
Contributor

@abendt abendt commented Mar 29, 2023

Some of the tests in arrow-core run slow on the non jvm platforms (#2908)

One reason seems to be the use of Arb.string() especially when used inside arbs that generate collections (map, list, nonemptylist)

See

This MR replaces Arb.string() with Arb.int() in some tests to speed up the execution time

abendt added 3 commits March 29, 2023 19:33
Arb.string() seems to be slow on iosX64, try to use Arb.unit() as we
don't really care about the type here
@abendt abendt changed the title try to speed up SequenceKTest try to speed up some Tests Mar 29, 2023
@abendt
Copy link
Contributor Author

abendt commented Mar 30, 2023

@abendt abendt changed the title try to speed up some Tests speed up some Tests Mar 30, 2023
@abendt abendt changed the title speed up some Tests Improve slow tests Mar 30, 2023
@abendt abendt changed the title Improve slow tests speedup 'Arb.string()' related slow tests Mar 30, 2023
@abendt abendt marked this pull request as ready for review March 30, 2023 10:19
@nomisRev
Copy link
Member

Wow that is an amazing improvements 😍 Thank you for digging into this @abendt!

A bit related, but also unrelated to this. I am a bit curious if running the tests on latest JDK would increase build times as well. IIRC only building the binaries in the publish step need to run in JDK 11? Related tweet, https://twitter.com/EsaFirm/status/1641083869083602944?s=20

Copy link
Member

@nomisRev nomisRev left a comment

Choose a reason for hiding this comment

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

Looks awesome @abendt. Just two small nits (probably missing proper imports, so suggestions is going to break).

@abendt
Copy link
Contributor Author

abendt commented Mar 30, 2023

A bit related, but also unrelated to this. I am a bit curious if running the tests on latest JDK would increase build times as well. IIRC only building the binaries in the publish step need to run in JDK 11? Related tweet, https://twitter.com/EsaFirm/status/1641083869083602944?s=20

Sure, that could prohably improve build times. More ideas:

@serras
Copy link
Member

serras commented Mar 30, 2023

Sure, that could prohably improve build times. More ideas:

We actually are working on improving that in #2988, although we may fix it once and for all in the Gradle plug-in we use arrow-kt/arrow-gradle-config#69

@abendt abendt changed the title speedup 'Arb.string()' related slow tests arrow-core: speedup 'Arb.string()' related slow tests Mar 30, 2023
Copy link
Collaborator

@franciscodr franciscodr left a comment

Choose a reason for hiding this comment

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

Thanks for helping us improve the build times, @abendt I appreciate the time you're putting on this!

@serras serras merged commit 1579e92 into arrow-kt:main Mar 30, 2023
@abendt abendt deleted the sequencek_test_performance branch March 30, 2023 19:02
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.

4 participants