Add native functions to benchmarks where they exist #1929

Merged
merged 2 commits into from Oct 30, 2016

Projects

None yet

4 participants

@bjdixon
Contributor
bjdixon commented Oct 6, 2016
  • Added benchmarks for either (addresses #1913)
  • Removed isSet benchmarks as isSet has been removed from ramda
  • Changed indexOf to includes in the contains benchmark as includes is
    semantically closer to contains than indexOf
@bjdixon bjdixon Add native functions to benchmarks where they exist
 - Added benchmarks for either (addresses #1913)
 - Removed isSet benchmarks as isSet has been removed from ramda
 - Changed indexOf to includes in the contains benchmark as includes is
semantically closer to contains than indexOf
3224857
@bjdixon
Contributor
bjdixon commented Oct 6, 2016

I'm not sure that the additions I've made in (filter|find|findIndex)-where.bench.js are helpful as they mix native and ramda functions. In terms of benchmarking in general I would have thought it better to have a separate where.bench.js.

I'd like to create further PRs with benchmarks for the remaining unbenchmarked ramda functions (with native comparisons where appropriate) but will wait on feedback from this PR so I better understand the direction these should take.

lib/bench/either.bench.js
+ name: 'either',
+ tests: {
+ 'either(gt10, even, 101)': function() {
+ R.either(gt10, even, 101)
@kedashoe
kedashoe Oct 6, 2016 Contributor

Either has an arity of 2 and returns a function so calling it like this doesn't do what it looks like.

@bjdixon
bjdixon Oct 7, 2016 Contributor

😳 oops

lib/bench/either.bench.js
+ R.either(gt10)(even)(101);
+ },
+ 'either(gt10, even, 8)': function() {
+ R.either(gt10, even, 8)
@kedashoe
kedashoe Oct 6, 2016 Contributor

⬆️

lib/bench/contains.bench.js
@@ -16,8 +16,8 @@ module.exports = {
'contains(c, cs)': function() {
return contains(randomLowerAlpha(), alphabet);
},
- 'native indexOf': function() {
- return nativeIndexOf(randomLowerAlpha(), alphabet);
+ 'native includes': function() {
@kedashoe
kedashoe Oct 6, 2016 Contributor

I think it makes more sense to call this 'native contains' even if the native function we use is not called contains (either way definitely better than indexOf from before).

@kedashoe
Contributor
kedashoe commented Oct 6, 2016

Thanks for the PR, the benchmark stuff could definitely use some ❤️

I'm not sure that the additions I've made in (filter|find|findIndex)-where.bench.js are helpful as they mix native and ramda functions. In terms of benchmarking in general I would have thought it better to have a separate where.bench.js.

Agreed, not sure we need native comparisons in benchmarks that test more than one ramda function.

In general I think it would be nice to have separate Ramda vs Ramda and Ramda vs native vs etc suites. Ramda vs others is interesting every now and then, but usually I just care if the changes I'm making make ramda itself faster or slower. Would also be nice to auto-compare a branch vs master.

@buzzdecafe
Member

In general I think it would be nice to have separate Ramda vs Ramda and Ramda vs native vs etc

👍 for this idea -- although it implies we would track benches over time?

@buzzdecafe buzzdecafe merged commit 1871cee into ramda:master Oct 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@CrossEye
Member

Wow, I missed this one entirely! Very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment