Improve performance of sortBy #92

paldepind · 2018-03-19T10:14:53Z

This PR improves the performance of sortBy and thus also sort. These are the benchmark results I get when running sort on an array of 100 numbers.

┌───────────────────────┬────────────────────┬──────────────────────┬────────┬───────────┬───┐
│ suite                 │ old                │ new                  │ diff   │ change    │ α │
├───────────────────────┼────────────────────┼──────────────────────┼────────┼───────────┼───┤
│ methods.sort.Array    │ 3 Hz ±1.49% (n 12) │ 637 Hz ±0.30% (n 89) │ 099.1% │ +21464.1% │ ✓ │
├───────────────────────┼────────────────────┼──────────────────────┼────────┼───────────┼───┤
│ methods.sort.Foldable │ 4 Hz ±0.99% (n 14) │ 317 Hz ±0.45% (n 82) │ 097.7% │ +8503.4%  │ ✓ │
└───────────────────────┴────────────────────┴──────────────────────┴────────┴───────────┴───┘

In this case sorting an array got 214 times faster and sorting a foldable (using List as the benchmark subject) got 85 times faster 😄

Note however that to achieve the performance increase I had to use fewer sanctuary-type-class functions internally in the implementation of sortBy. It seems that using sanctuary-type-class functions results in some overhead. I created two benchmarks to try and measure the overhead of the functions used inside sortBy.

var test = R.range(0, 100);
// ...
'methods.test.reduceMin':     function(Z) { Z.reduce(Z.min, 0, test); },
'methods.test.append':        function(Z) { Z.reduce((a, b) => Z.append(b, a), [], [0, 1, 2, 3, 4]); },

Then I plugged in Ramda and sanctuary-type-class in the above benchmarks and got the following results:

┌───────────────────────┬──────────────────────────┬────────────────────────────┬────────┬───────────┬───┐
│ suite                 │ sanctuary-type-classes   │ Ramda                      │ diff   │ change    │ α │
├───────────────────────┼──────────────────────────┼────────────────────────────┼────────┼───────────┼───┤
│ methods.test.reduceMin│ 2,073 Hz ±0.52% (n 89)   │ 504,551 Hz ±0.37% (n 93)   │ 099.2% │ +24235.2% │ ✓ │
├───────────────────────┼──────────────────────────┼────────────────────────────┼────────┼───────────┼───┤
│ methods.test.append   │ 235,364 Hz ±1.02% (n 87) │ 1,022,981 Hz ±0.46% (n 91) │ 062.6% │ +334.6%   │ ✓ │
└───────────────────────┴──────────────────────────┴────────────────────────────┴────────┴───────────┴───┘

Getting the minimum of an array of 100 numbers seems to be 242 times as slow with sanctuary-type-classes and creating an array of size 5 by prepending is about 3.3 times slower than Ramda. Z.lte in particular seems to be be slow and it is still used inside sortBy in some cases and in these cases sortBy is still slow due to Z.lte ending up as the bottleneck.

Avaq

It gives me a warm feeling on the inside to see my hard work on the benchmarks being put to good use! I found one potential improvement.

Avaq · 2018-03-19T11:11:56Z

index.js

+    }
+    var lteFn =
+      typeof rs[0].prop === 'string' || typeof rs[0].prop === 'number' ?
+        function(a, b) { return a <= b; } :


This function can be hoisted. May slightly benefit performance.

The function above behaves incorrectly for NaN, as this demonstrates:

> Z.sort ([NaN, 2, NaN, 0, NaN, 1, NaN]) [NaN, NaN, 0, 1, NaN, 2, NaN]

We must either remove the special case for typeof rs[0].prop === 'number' or ensure via other means that NaN is handled appropriately.

Z.lte was only recently updated to handle NaN, in #88.

The fast-path for numbers and strings is very important for performance.

I think we should consider whether it's worth the code complexity and performance to handle NaN. For instance, Array$prototype$lte doesn't handle sparse arrays because sparse arrays can be considered to not be a part of the array type we define. In the same way Number$prototype$lte could not handle NaN as NaN could be considered not part of the number type we define.

I think it would be best for these changes to be refactoring only. In a subsequent pull request you could make the case for not supporting NaN by showing the performance difference.

davidchambers · 2018-03-19T11:57:38Z

Amazing work, Simon!

davidchambers · 2018-03-19T11:59:47Z

index.js

+    var rs = reduce(reducer, [], foldable);
+    if (rs.length === 0) {
+      return foldable;
+    }


Does this special case make an appreciable difference? I would have thought that sorting an empty structure would be an efficient operation.

I now see that this is guarding rs[0].prop below. I would still like to remove the special case. We could define lteFn like so:

var type = rs.length > 0 ? typeof rs[0].prop : 'undefined'; var lteFn = type === 'number' || type === 'string' ? ... : lte;

davidchambers · 2018-03-19T12:02:25Z

index.js

+    var foldableIsArray = Array.isArray(foldable);
+    var idx = 0;
+    function reducer(xs, x) {
+      xs.push({idx: idx, x: x, prop: f(x)});


Do you consider prop to be a better name than fx for the f(x) property?

I'm indifferent. For prop I didn't actually look at the old name. The thought behind prop is that f returns some property of x (not property as in a JS property of an object but in the more general sense of the word). But I'd have no problem in switching it back.

I prefer fx. :)

davidchambers · 2018-03-19T12:25:54Z

index.js

+        lteFn(b.prop, a.prop) ? a.idx - b.idx : -1 :
+        1;
+    });
+    if (foldableIsArray) {


Since foldableIsArray is only referenced here, let's replace this identifier with Array.isArray(foldable).

Good idea. I was using it in several places earlier but that went away.

davidchambers · 2018-03-19T12:27:30Z

index.js

+      return rs;
+    } else {
+      var result = empty(foldable.constructor);
+      var of = foldable.constructor['fantasy-land/of'];


Is this safe? Could foldable.constructor be Function?

davidchambers · 2018-03-19T12:30:10Z

index.js

+      var result = empty(foldable.constructor);
+      var of = foldable.constructor['fantasy-land/of'];
+      for (var i = 0; i < rs.length; i += 1) {
+        result = result['fantasy-land/concat'](of(rs[i].x));


Is this safe? Are we sure that Array is the only built-in type that can support sorting?

davidchambers · 2018-03-19T12:31:07Z

package.json

@@ -19,6 +19,9 @@
  },
  "devDependencies": {
    "fantasy-land": "3.5.0",
+    "knuth-shuffle": "^1.0.8",
+    "list": "^2.0.7",
+    "ramda": "^0.25.0",


Please use the "1.2.x" form rather than the "^1.2.3" form.

davidchambers · 2018-03-19T12:32:04Z

index.js

+    for (var i = 0; i < this.length; i += 1) {
+      acc = f(acc, this[i]);
+    }
+    return acc;
  }


Does this change make an appreciable difference?

I've measured the difference in the benchmark suite for List. I'd say the difference is quite significant.

Significant indeed!

Now that I know we'll keep the code I'll share my formatting request. I'd like to see this:

for (var idx = 0; idx < this.length; idx += 1) acc = f(acc, this[idx]);

paldepind · 2018-03-19T13:29:12Z

@Avaq Props on the benchmark system! The output in particular is really nice 👍

davidchambers · 2018-03-25T20:26:47Z

@paldepind, I'm happy to push tweaks to your branch. I'd love to merge this pull request soon. :)

paldepind · 2018-03-26T07:19:54Z

@davidchambers You're very welcome to that David 👍

paldepind · 2018-03-27T10:33:20Z

Looks good to me @davidchambers.

paldepind · 2018-03-27T10:36:19Z

index.js

@@ -2132,9 +2132,7 @@
      return xs;
    }
    var rs = reduce(reducer, [], foldable);
-    if (rs.length === 0) return foldable;
-
-    var type = typeof rs[0].fx;


What is the benefit of this? Replacing one special case with another that doesn't terminate as early?

I prefer typeof (rs[0] && rs[0].fx) because the unsafe expression (rs[0].fx) and its guard appear very close together. My preference is not strong, though, so feel free to reinstate the rs.length === 0 check. :)

paldepind · 2018-03-27T10:39:35Z

index.js

+    for (idx = 0; idx < rs.length; idx += 1) {
+      result = concat(result, of(F, rs[idx].x));
+    }
+    return result;


This does seem to affect performance. On my computer, the Foldable benchmark went from 308 Hz to 233 Hz. That is, the previous is about 33% faster.

I'm concerned about correctness. Representatives of built-in types do not have fantasy-land/-prefixed members, so replacing concat(x, y) with x['fantasy-land/concat'](y) is not necessarily valid. Can we be sure that result will have a fantasy-land/concat member and that F will have a fantasy-land/of member?

fantasy-land/concat implementations are provided for the following built-in types: String, Array, and Object.

fantasy-land/of implementations are provided for the following built-in types: Array and Function.

Are any of the build-in types, except for Array, both a foldable and a monoid?

As a StrMap, Object also implements Foldable and Monoid.

@gabejohnson Sorry, I forgot to mention that it has to be an Applicative as well.

Ahhh...I missed the of. Yeah, Array is the only one that fits the bill AFAIK.

If we reinstate the ['fantasy-land/concat'] and ['fantasy-land/of'] optimizations, let's add a comment explaining why these optimizations are safe.

davidchambers · 2018-04-01T10:35:15Z

ℹ️ @paldepind, I rebased this branch.

davidchambers · 2018-04-01T10:50:30Z

package.json

@@ -19,6 +19,7 @@
  },
  "devDependencies": {
    "fantasy-land": "3.5.0",
+    "list": "2.0.7",


@Avaq, should this be listed here or in bench/package.json?

It's used as a library like any other (just so happens to be used in benchmarks), so it should be listed here. The bench/package.json file only exists to allow for the installation of any version of choice, of the package you're benchmarking against.

paldepind · 2018-04-02T08:49:56Z

LGTM 🎉

Avaq reviewed Mar 19, 2018

View reviewed changes

davidchambers reviewed Mar 19, 2018

View reviewed changes

paldepind commented Mar 27, 2018

View reviewed changes

davidchambers mentioned this pull request Mar 27, 2018

assert that ‘sort’ and ‘sortBy’ do not rely on Array#sort being stable #94

Merged

davidchambers reviewed Apr 1, 2018

View reviewed changes

Improve performance of sortBy

9c5744e

davidchambers merged commit 90baa0e into sanctuary-js:master Apr 29, 2018

snyk-bot mentioned this pull request Mar 31, 2020

[Snyk] Upgrade sanctuary-type-classes from 8.1.1 to 8.2.1 dmitriz/fantasy-laws#1

Open

Improve performance of sortBy #92

Improve performance of sortBy #92

Conversation

paldepind commented Mar 19, 2018

Avaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paldepind commented Mar 19, 2018

davidchambers commented Mar 25, 2018

paldepind commented Mar 26, 2018

paldepind commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paldepind commented Apr 2, 2018