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

Fix for issue 382 #383

Closed
wants to merge 1 commit into from
Closed

Fix for issue 382 #383

wants to merge 1 commit into from

Conversation

lemire
Copy link
Member

@lemire lemire commented Apr 7, 2020

With this patch, running a single short test with gradle takes 21 second on a powerful iMac. That's still an order of magnitude longer than it should take, but a hell of a lot better than what we currently have in master... where it can take minutes...

$ time ./gradlew :roaringbitmap:test --tests *testIndexIterator4
Starting a Gradle Daemon (subsequent builds will be faster)

> Task :RoaringBitmap:compileTestJava
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

BUILD SUCCESSFUL in 21s
7 actionable tasks: 2 executed, 5 up-to-date

real	0m21.967s
user	0m1.118s
sys	0m0.147s

If you think that this is not necessary, please tell me how long ./gradlew :roaringbitmap:test --tests *testIndexIterator4 takes to run for you. Please don't tell me that it is fine in some IDE that is configured in a certain way... this should work right out of the box in gradle.

Fixes #382

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

It is pathetic that it still takes seconds to run a single test.

@coveralls
Copy link

coveralls commented Apr 7, 2020

Coverage Status

Coverage remained the same at 91.517% when pulling e4e3051 on dlemire/fixing382 into aaac8cc on master.

@richardstartin
Copy link
Member

I don’t know what the issue is, but will look at the issue tomorrow morning. The fact that the “slow” tests run in milliseconds or minutes depending on the runner points to the runner, in my opinion. It’s likely a build configuration causing what you’re seeing. Please note in the meantime that if you want to do comparisons, you need to kill the gradle daemon between runs to do a fair test.

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Please note in the meantime that if you want to do comparisons, you need to kill the gradle daemon between runs to do a fair test.

It is the difference between seconds and... well, mining bitcoins.

@richardstartin
Copy link
Member

It is the difference between seconds and... well, mining bitcoins.

Yes, but I am sure you are measuring the time it takes to start up a gradle daemon. As I said, with a different runner, the part of the whole you are fixing takes milliseconds. So what’s different?

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

I am going to close and reopen because travis is busted.

@lemire lemire closed this Apr 7, 2020
@lemire lemire reopened this Apr 7, 2020
@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

And travis wakes up...

@richardstartin
Copy link
Member

Looks like it's related to this.

@richardstartin
Copy link
Member

I can see the behaviour you're reporting, it took 7 minutes before I killed the build, but compared to the milliseconds or 15 seconds it takes in the IDE, it's obviously a gradle issue... I will investigate.

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Looks like it's related to this.

This refers to an issue fixed in gradle 4. I am running gradle 5. It might very well be the same issue, but if it is, they closed it too soon.

@richardstartin
Copy link
Member

Yes, I think we've established that command is slow. I am trying to upgrade gradle, but it requires irritating checkstyle changes. Could you try -Dtest.single?

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Could you try -Dtest.single?

It has been removed.

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

@richardstartin If there was an easy workaround, I would have simply complained about gradle and moved on, but to my knowledge, there is just one way, using gradle, to run specific tests, and that's the way I am describing.

@richardstartin
Copy link
Member

@lemire If you anchor the prefix of the test e.g.

./gradlew :roaringbitmap:test --tests RoaringBitmapSubsetTest.*

or

./gradlew --profile :roaringbitmap:test --tests TestIterators.testIndexIterator4

The tests execute in under 5 seconds. I think you have found an inefficiency in the way the unanchored prefix scan works. This is a bug, and paramterised tests are very useful, and efficient, except for when you encounter a performance bug in the interaction between two libraries.

In any case, I have upgraded to gradle 6.3, along with the mandatory checkstyle changes.

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Great work.

@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Closing without merging.

@lemire lemire closed this Apr 7, 2020
@lemire
Copy link
Member Author

lemire commented Apr 7, 2020

Someone (not me) should report this to gradle because it is a pretty terrible bug.

@lemire lemire deleted the dlemire/fixing382 branch June 14, 2021 13:37
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.

Testing performance when doing a single test is super slow
3 participants