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

Add benchmarks for Nullable #24

Merged
merged 1 commit into from
Sep 18, 2016
Merged

Conversation

nalimilan
Copy link
Collaborator

Includes in particular a reduced version of NullableArrays to measure loop
performance.

This is my first experience with this package, so please give my advice on what benchmarks should look like.

@KristofferC
Copy link
Contributor

I would use the samerand function instead of just seeding it in case more benchmarks to this file are added in between the current ones.


const SUITE = BenchmarkGroup()

srand(1)
Copy link
Member

@jrevels jrevels Sep 16, 2016

Choose a reason for hiding this comment

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

Like @KristofferC mentioned, you can do using .RandUtils and then replace uses of rand in this module with samerand.

@jrevels
Copy link
Member

jrevels commented Sep 16, 2016

Thanks for the contribution, the more benchmarks the merrier!

Since the benchmark parameters here don't yet exist in etc/params.jld (that file is populated during the tuning process, which has to be executed on Nanosoldier), the method which loads the parameters into the module's BenchmarkGroup is failing.

I should make that method less fragile...let me fix that, then you can rebase this PR onto the fix so that tests here pass.

EDIT: Done with the fix. Feel free to rebase this against master and see if the tests pass.

@nalimilan
Copy link
Collaborator Author

Thanks! I've just rebased against master, used samerand as advised, and improved a few things in the benchmarks.

I'd still like to hear @johnmyleswhite and @DavidGold's comment about these. FWIW, I've just checked that perf_sum and perf_countnulls use SIMD (though perf_countequals doesn't, even after tweaking isequal).

@nalimilan
Copy link
Collaborator Author

Oh, and it is fine/recommended to test so many types in a loop like I do? If that takes too much time, I can keep only a subset of them.

@nalimilan nalimilan force-pushed the nl/nullable branch 2 times, most recently from 1c5d318 to 1f8452b Compare September 16, 2016 21:57
@davidagold
Copy link

Thanks for doing this, Milan. Will this be coordinated with JuliaLang/julia#18510 ?

I can think of more benchmarks for NullableArrays. In particular, we should have benchmarks for iterating over zipped NullableVectors. I'll see about modernizing NullableArrays/perf.

@nalimilan
Copy link
Collaborator Author

Thanks for doing this, Milan. Will this be coordinated with JuliaLang/julia#18510 ?

The idea was that if we merge these benchmarks first, they will allow checking that the move to hasvalue doesn't affect performance. Then we can also apply the same change to the isnull field of NullableArray.

I can think of more benchmarks for NullableArrays. In particular, we should have benchmarks for iterating over zipped NullableVectors. I'll see about modernizing NullableArrays/perf.

Could you give an example of what you mean? Is that just something similar to perf_countequals, but using zip instead of eachindex? Anyway, once we have more operators on Nullable in base, we'll be able to add more benchmarks.

@jrevels
Copy link
Member

jrevels commented Sep 18, 2016

Oh, and it is fine/recommended to test so many types in a loop like I do? If that takes too much time, I can keep only a subset of them.

Should be fine. The majority of turnaround time for Nanosoldier jobs is actually building Julia rather than running benchmarks.

The idea was that if we merge these benchmarks first, they will allow checking that the move to hasvalue doesn't affect performance.

Just making sure you're aware - benchmarks merged here won't immediately be available via Nanosoldier, as they have to undergo a tuning process (which I can trigger tomorrow, if this PR is merged by then).

Other than triggering the process and committing the results, the tuning process is pretty automated. When I get the chance, I should probably set up a cron job that does this nightly...

@jrevels
Copy link
Member

jrevels commented Sep 18, 2016

Also, let me know once you're ready for me to merge this and I'll do so

@nalimilan
Copy link
Collaborator Author

Thanks. Should be good to merge now. It would be nice if you could run it on Nanosoldier, as it would make a reference point for current PRs against Base.

Includes in particular a reduced version of NullableArrays to measure loop
performance.
@jrevels jrevels merged commit 83888b9 into JuliaCI:master Sep 18, 2016
@nalimilan nalimilan deleted the nl/nullable branch September 18, 2016 20:59
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