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

remove sliced #11238

Merged
merged 1 commit into from Jan 20, 2022
Merged

remove sliced #11238

merged 1 commit into from Jan 20, 2022

Conversation

Uzlopak
Copy link
Collaborator

@Uzlopak Uzlopak commented Jan 18, 2022

Remove the package "sliced" in favor of native functions.

@Uzlopak Uzlopak closed this Jan 18, 2022
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jan 18, 2022

Closing for now, as I am unsure if this has a perf impact.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jan 18, 2022

Here some benchmarks on comparing spread-operator, Array.from and sliced

var sliced = require('./')
var Bench = require('benchmark');
var s = new Bench.Suite;
var slice = [].slice;

var testArray = new Array(10).fill("a")


s.add('[...]', function () {
  [...testArray];
}).add('Array.from', function () {
  Array.from(testArray);
}).add('Array.prototype.slice.call(testArray)', function () {
  Array.prototype.slice.call(testArray);
}).add('cached slice.call', function () {
  slice.call(testArray)
}).add('sliced', function () {
  sliced(testArray)
}).on('cycle', function (evt) {
  console.log(String(evt.target));
}).on('complete', function () {
  console.log('fastest is %s', this.filter('fastest').pluck('name'));
})
.run();

with 10 elements

[...] x 33,485,060 ops/sec ±1.69% (84 runs sampled)
Array.from x 25,203,273 ops/sec ±0.55% (96 runs sampled)
Array.prototype.slice.call(testArray) x 30,466,867 ops/sec ±0.58% (97 runs sampled)
cached slice.call x 31,882,883 ops/sec ±0.64% (94 runs sampled)
sliced x 18,666,861 ops/sec ±0.94% (94 runs sampled)
fastest is [ '[...]' ]

with 1000 elements:

[...] x 510,025 ops/sec ±0.85% (94 runs sampled)
Array.from x 538,160 ops/sec ±1.06% (91 runs sampled)
Array.prototype.slice.call(testArray) x 887,796 ops/sec ±0.63% (99 runs sampled)
cached slice.call x 949,157 ops/sec ±1.06% (97 runs sampled)
sliced x 308,079 ops/sec ±0.53% (99 runs sampled)
fastest is [ 'cached slice.call' ]

So [...] and Array.from are about the same speed. slice.call is not usable because arguments is not an array but an iteratable. But in any case sliced is slower than the built-ins.

@@ -173,7 +172,7 @@ function iter(i) {
if (debug) {
if (typeof debug === 'function') {
debug.apply(_this,
[_this.name, i].concat(sliced(args, 0, args.length - 1)));
[_this.name, i].concat(args.slice(0, args.length - 1)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here i used normal slice, as I think debug is not used in production?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug can be used in production. I'm sure some people use it for logging or perf. Shouldn't assume that certain features are never used in prod.

@Uzlopak Uzlopak reopened this Jan 18, 2022
@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jan 18, 2022

Also

var sliced = require('./')
var Bench = require('benchmark');
var s = new Bench.Suite;
var slice = [].slice;

var testArray = new Array(3).fill("a")


s.add('direct', function () {
  [testArray[0], testArray[1], testArray[2]];
}).add('[...]', function () {
  [...testArray];
}).add('Array.from', function () {
  Array.from(testArray);
}).add('Array.prototype.slice.call(testArray)', function () {
  Array.prototype.slice.call(testArray);
}).add('cached slice.call', function () {
  slice.call(testArray)
}).add('sliced', function () {
  sliced(testArray)
}).on('cycle', function (evt) {
  console.log(String(evt.target));
}).on('complete', function () {
  console.log('fastest is %s', this.filter('fastest').pluck('name'));
})
.run();
direct x 2,804,270,988 ops/sec ±0.83% (100 runs sampled)
[...] x 47,105,492 ops/sec ±0.88% (92 runs sampled)
Array.from x 31,716,899 ops/sec ±0.92% (93 runs sampled)
Array.prototype.slice.call(testArray) x 36,296,758 ops/sec ±0.80% (94 runs sampled)
cached slice.call x 36,020,585 ops/sec ±0.71% (94 runs sampled)
sliced x 28,710,453 ops/sec ±1.06% (91 runs sampled)
fastest is [ 'direct' ]

So if we know that the length of the array is 2 or 3, than directly assigning the values to the new Array is much faster...

@@ -5,7 +5,7 @@

'use strict';

const utils = require('./utils');
const utils = require('./utils'); // eslint-disable-line no-unused-vars
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont know why removing this line makes the unit tests fail.

@Uzlopak
Copy link
Collaborator Author

Uzlopak commented Jan 18, 2022

I checked mongoose sourcecode. It seems that the spread operator is not used usually, but everywhere Array.from. I can change the occurences of the spread operator to Array.from if it is desired.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Nice work, thanks 👍 I like removing the extra dependency, especially if the benchmarks that justified using sliced are no longer accurate.

@vkarpov15 vkarpov15 added this to the 6.1.8 milestone Jan 20, 2022
@vkarpov15 vkarpov15 merged commit c609e5c into Automattic:master Jan 20, 2022
@AbdelrahmanHafez
Copy link
Collaborator

Nice work, thanks @Uzlopak 👍

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.

None yet

3 participants