-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
4efd577
to
64ecf53
Compare
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 |
@ararslan would you mind trying with this new version? |
I won't have time for a bit, but you can run the same code that Nanosoldier does: it's in this repo, |
|
||
# return a collection that can serve to initialize a container C of type T | ||
initcoll(C, T) = let initmap = Dict(Vector => Vector, | ||
Dict => Pair, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
64ecf53
to
5d09dad
Compare
@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. |
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. |
That would be great, but really no worries if you have too little time, there is no reason that I put pressure on you! |
Awesome, the PR is passing CI again, so just let me know how the tuning script goes. |
Tuning passes on my end! |
Excellent, thanks for checking! |
Thanks for all your thankless work! fingers crossed no... |
DO NOT MERGEThis is the same as #109, just to check if it can pass CI (it causes problem with tuning).