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

Activity

1. starkwang

   added

   help wantedIssues that need assistance from volunteers or PRs that need help to proceed.Issues that need assistance from volunteers or PRs that need help to proceed.

   good first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.

   on Mar 10, 2019
2. mscdex commented on Mar 10, 2019

   

   mscdex

   on Mar 10, 2019

   Contributor

   More actions

   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.
3. starkwang commented on Mar 10, 2019

   

   starkwang

   on Mar 10, 2019

   ContributorAuthor

   More actions

   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.

   👍React with 👍1Reacted by Rizky Luthfianto
4. BridgeAR commented on Mar 10, 2019

   

   BridgeAR

   on Mar 10, 2019

   Member

   More actions

   @starkwang the actual code is optimized away in your benchmark. Therefore it is significantly faster.
5. BridgeAR commented on Mar 10, 2019

   

   BridgeAR

   on Mar 10, 2019

   Member

   More actions

   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.

   👍React with 👍1Reacted by Alex Yang
6. BridgeAR

   removed

   good first issueIssues that are suitable for first-time contributors.Issues that are suitable for first-time contributors.

   help wantedIssues that need assistance from volunteers or PRs that need help to proceed.Issues that need assistance from volunteers or PRs that need help to proceed.

   on Mar 10, 2019
7. BridgeAR commented on Mar 10, 2019

   

   BridgeAR

   on Mar 10, 2019

   Member

   More actions

   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
   
8. devsnek commented on Mar 10, 2019

   

   devsnek

   on Mar 10, 2019

   Last edited by devsnek

   Member

   More actions

   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

   👍React with 👍2Reacted by Refael Ackermann and Ilya
9. BridgeAR commented on Mar 10, 2019

   

   BridgeAR

   on Mar 10, 2019

   Last edited by BridgeAR

   Member

   More actions

   @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.
10. refack commented on Mar 10, 2019

    

    refack

    on Mar 10, 2019

    Last edited by refack

    Contributor

    More actions

    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

    👍React with 👍10Reacted by Weijia Wang, ad737079, Ilya, Rizky Luthfianto, antsmartian, ZYSzys, Denys Otrishko, GrosSacASacs, Rouven Weßling and Vitali Kuzmich
11. starkwang

    added a commit that references this issue on Mar 21, 2019

    lib: use Array#includes instead of Array#indexOf

    ...

    c8d3a73
12. starkwang

    closed this as completedon Mar 26, 2019