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 some collection operations #150

Merged
merged 1 commit into from
Dec 15, 2017

Conversation

rfourquet
Copy link
Contributor

@rfourquet rfourquet commented Dec 9, 2017

DO NOT MERGE
This is the same as #109, just to check if it can pass CI (it causes problem with tuning).

@rfourquet
Copy link
Contributor Author

rfourquet commented Dec 9, 2017

So this is what I suspected: CI passes, even if there is an underlying problem which pops up only when re-tuning. So I updated the offending line by setting evals to the number of elements in the collection before pop!ing values, hoping this will be enough. Removing WIP, as I don't see what to do else to be sure it will work, besides trying.

@rfourquet rfourquet changed the title WIP: add benchmarks for some collection operations add benchmarks for some collection operations Dec 9, 2017
@rfourquet
Copy link
Contributor Author

@ararslan would you mind trying with this new version?

@ararslan
Copy link
Member

I won't have time for a bit, but you can run the same code that Nanosoldier does: it's in this repo, etc/tune.jl.


# return a collection that can serve to initialize a container C of type T
initcoll(C, T) = let initmap = Dict(Vector => Vector,
Dict => Pair,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can ObjectIDict (and maybe WeakKeyDict) be added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but as it's not parametric like Dict, it will be a little bit of work to include, so I won't have time to do that in this PR. If you want to open a PR based on this one, ping me and I will review.

@rfourquet
Copy link
Contributor Author

@ararslan Thanks for the trick with etc/tune.jl ! Inded it needed a last update, but now it passes on my machine. If CI turns green, would you make me the favor to mege and retune Nanosoldier? It would be great to run this benchmark on two PR I hope to see merged before the feature freeze.

@ararslan
Copy link
Member

Looks like JSON needs an update for 0.7, since Unicode was just moved to the stdlib. Nanosoldier won't work without that, so I'll try to get that in ASAP.

@rfourquet
Copy link
Contributor Author

so I'll try to get that in ASAP.

That would be great, but really no worries if you have too little time, there is no reason that I put pressure on you!

@ararslan
Copy link
Member

Awesome, the PR is passing CI again, so just let me know how the tuning script goes.

@rfourquet
Copy link
Contributor Author

Tuning passes on my end!

@ararslan ararslan merged commit dff07d9 into JuliaCI:master Dec 15, 2017
@ararslan
Copy link
Member

Excellent, thanks for checking!

@rfourquet
Copy link
Contributor Author

Thanks for all your thankless work! fingers crossed no...

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.

3 participants