Node: lib: prefer using Array#includes instead of Array#indexOf

[starkwang]

In our lib code, we are still using Array#indexOf to determine if an item is existed in array.

For example:
https://github.com/nodejs/node/blob/3414bc7b25a00556af80c6ba946e28464e7e8d36/lib/url.js#L584

https://github.com/nodejs/node/blob/b05fd4baa87886674721101eaf38b75716037891/lib/internal/util/inspect.js#L273

For semantics, I think it's more appropriate to move them into Array#includes and it seems no performance difference between them at present: https://jsperf.com/array-indexof-vs-includes.

[mscdex]

Assuming you tested the equivalent of master, did you also check the versioned branches? It may/may not be as fast in older versions of V8.

[starkwang]

I've tested it on LTS version (code). Here is the result:

Node.js 6.17.0 (indexOf is 3x faster):

Array#indexOf x 7,221,953 ops/sec ±2.65% (79 runs sampled)
Array#includes x 1,972,658 ops/sec ±2.02% (84 runs sampled)
Fastest is Array#indexOf

Node.js 8.15.1 (same):

Array#indexOf x 7,340,047 ops/sec ±4.30% (75 runs sampled)
Array#includes x 8,899,792 ops/sec ±0.44% (90 runs sampled)
Fastest is Array#includes

Node.js 10.15.3 (includes is 80x faster):

Array#indexOf x 11,671,636 ops/sec ±4.92% (73 runs sampled)
Array#includes x 861,062,120 ops/sec ±0.51% (91 runs sampled)
Fastest is Array#includes

Node.js 11.11.0 (includes is 80x faster):

Array#indexOf x 10,776,587 ops/sec ±4.10% (78 runs sampled)
Array#includes x 854,403,715 ops/sec ±0.60% (88 runs sampled)
Fastest is Array#includes

So we may not land this change into 6.x.

[BridgeAR]

@starkwang the actual code is optimized away in your benchmark. Therefore it is significantly faster.

[BridgeAR]

Array#indexOf() will always be faster as it has less to do by spec and the engine is not able to optimize that part away. The difference is not big but it is definitely there.

I do not think we should change this ever.

[BridgeAR]

const items = [
  'jpg', '3fr', 'ari', 'arw',
  'bay', 'crw', 'cr2', 'cap',
  'data', 'dcs', 'dcr', 'dng',
  'drf', 'eip', 'erf', 'fff',
  'iiq', 'k25', 'kdc', 'mdc',
  'mef', 'mos', 'mrw', 'nef',
  'nrw', 'obm', 'orf', 'pef',
  'ptx', 'pxn', 'r3d', 'raf',
  'raw', 'rwl', 'rw2', 'rwz',
  'sr2', 'srf', 'srw', 'tif',
  'x3f'
];

const searchStrings = [
  'raf',
  'nef',
  'cr'
];

function indexOf(arr, str) {
  if (arr.indexOf(str) !== -1)
    return 1;
  return 0;
}

function includes(arr, str) {
  if (arr.includes(str))
    return 1;
  return 0;
}

function bench(fn) {
  console.time(fn.name);
  let count = 0;
  for (var i = 0; i < 1e7; i++) {
    count += fn(items, searchStrings[i % searchStrings.length]);
  }
  console.timeEnd(fn.name);
  console.log(count);
}

for (var i = 0; i < 3; i++) {
  bench(indexOf);
  bench(includes);
}

Yields on master:

indexOf: 756.462ms
includes: 958.480ms
indexOf: 946.475ms
includes: 965.666ms
indexOf: 947.888ms
includes: 1002.555ms

[devsnek]

Array#indexOf() will always be faster as it has less to do by spec and the engine is not able to optimize that part away.

just for the record, includes is actually much simpler in the spec. indexOf does a HasProperty before each Get, while includes just goes straight to Get.

on v11.11.0 i get:

the difference is negligible

[BridgeAR]

@devsnek you are right, I remembered the spec wrong in this case (I remembered it the other way around). However, in most cases the HasProperty check will just be optimized away by the engine.

V8 7.3 seems to be on par performance wise.

[refack]

For such a tiny op, depend on the use case, IMHO we should ___always___ prefer readability, simplicity, and explicivity. If the usage is to simply test for inclusion, and there is no use of the found index, then .include is optimal.

One main reason for this is that our stdlib is used as a reference implementation.

https://jsperf.com/array-indexof-vs-includes/25 that indicate +/- 10% on my chrome's V8 7.2