Use `Array.from` instead of `Array.prototype.slice.call` #1722

Korilakkuma · 2018-05-19T13:27:47Z

This PR will...

Use Array.from instead of Array.prototype.slice.call.

Why is this Pull Request needed?

This PR is to use better function than Array.prototype.slice.call.

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

changes have been done against master branch, and PR does not conflict
no commits have been done in dist folder (we will take care of updating it)
new unit / functional tests have been added (whenever applicable)
API or design changes are documented in API.md

- Use rest parameters - Use `Array.from`

johnBartos · 2018-05-19T14:53:16Z

We can’t use the rest operator because it’s not being polyfilled. We don’t polyfill it because it makes the bundle larger, and .slice works fine

Korilakkuma · 2018-05-19T15:03:43Z

Sorry ... I'm going to remove rest parameters.
But, please let me ask a question. For example, the following code seems using `rest parameters'. Is this mistaken ?

https://github.com/video-dev/hls.js/blob/master/src/event-handler.js#L18

azu · 2018-05-19T15:29:43Z

Use Array.from instead of Array.prototype.slice.call

The application code can use Array.from instead of Array.prototype.slice.call.
Because, application code can use polyfill library.

But, The library(hls.js) code should not useArray.from.
Because, the library should not bundle polyfill and the library should not introduce unneeded feature detect.
Additionally, Array.from is high cost than Array.prototype.slice at least in principle.

Does your library provide a good experience only when the feature is available natively?
-- https://w3ctag.github.io/polyfills/#advice-for-library-and-framework-authors

I think that this PR require benchmark.

azu · 2018-05-19T15:36:42Z

rest parameters

rest parameters is transpiled by babel in hls.js codebase

Caveats · Babel

Korilakkuma · 2018-05-19T15:41:07Z

Additionally, Array.from is high cost than Array.prototype.slice at least in principle.

Thanks. I don't know this principle (Rather, I think that Array.from is faster than Function.prototype.call) ...

johnBartos · 2018-05-19T16:04:08Z

Ah I guess we do!

Korilakkuma · 2018-05-19T16:43:58Z

I tried benchmarking by the following code ...

(function(){
  const UINT32_MAX = Math.pow(2, 32);

  function randomArrayLike(length) {
    const a = [];

    for (let i = 0; i < length; i++) {
       a.push(Math.floor(Math.random() * UINT32_MAX));
    }

    return new Uint32Array(a);
  }

  const loop = 32768;
  const length = 16384;

  function benchSliceCall(arraylike) {
    for (let i = 0; i < loop; i++) {
      Array.prototype.slice.call(arraylike, 0);
    }
  }

  function benchFrom(arraylike) {
    for (let i = 0; i < loop; i++) {
      Array.from(arraylike);
    }
  }

  let arraylike = randomArrayLike(length);

  console.time('slice.call1');
  benchSliceCall(arraylike);
  console.timeEnd('slice.call1');

  console.time('from1');
  benchFrom(arraylike);
  console.timeEnd('from1');

  arraylike = randomArrayLike(length);

  console.time('from2');
  benchFrom(arraylike);
  console.timeEnd('from2');

  console.time('slice.call2');
  benchSliceCall(arraylike);
  console.timeEnd('slice.call2');
})();

The results is the following ...

slice.call1: 131703.42993164062ms
from1: 101884.7890625ms
from2: 93370.76489257812ms
slice.call2: 118715.61010742188ms

Korilakkuma · 2018-05-19T16:45:30Z

This shows a somewhat difference ... Therefore, I think that the browsers that can use native Array.from might as well use it ... but, I'm not sure ...

azu · 2018-05-20T03:15:14Z

I've modified a bit #1722 (comment).
(prevent eliminate result of copy)

(function() {
    const UINT32_MAX = Math.pow(2, 32);

    function noopPreventOptimizeVM(arg) {
        return function(){
            return arg;
        };
    }

    function randomArrayLike(length) {
        const a = [];

        for (let i = 0; i < length; i++) {
            a.push(Math.floor(Math.random() * UINT32_MAX));
        }

        return new Uint32Array(a);
    }

    const loop = 32768;
    const length = 16384;

    function benchSliceCall(arraylike) {
        let results = [];
        for (let i = 0; i < loop; i++) {
            const copy = Array.prototype.slice.call(arraylike, 0); // prevent eliminate
            results.push(copy.length);
        }
        return results;
    }

    function benchFrom(arraylike) {
        let results = [];
        for (let i = 0; i < loop; i++) {
            const copy = Array.from(arraylike); // prevent eliminate
            results.push(copy.length);
        }
        return results;
    }

    let arraylike1 = randomArrayLike(length);

    const startSlice = Date.now();
    const r1 = benchSliceCall(arraylike1);
    noopPreventOptimizeVM(r1.length);
    print("slice:");
    print(Date.now() - startSlice);

    let arraylike2 = randomArrayLike(length);
    const startFrom = Date.now();
    const r2 = benchFrom(arraylike2);
    noopPreventOptimizeVM(r2.length);
    print("from:");
    print(Date.now() - startFrom);
})();

Run the microbench on jsvu + eshost.

$ eshost microbench.js
#### JavaScriptCore
slice:
23201
from:
18566

#### SpiderMonkey
slice:
67712
from:
24012

#### Chakra
slice:
22809
from:
139586

#### V8
slice:
155402
from:
61875

#### V8 --harmony
slice:
156824
from:
64353

loop count: 100 result. JSC does not optimize Array.from

#### JavaScriptCore slice: 85 from: 190

SpiderMonkey

slice:
236
from:
85

Chakra

slice:
81
from:
485

V8 --harmony

slice:
490
from:
177

V8

slice:
495
from:
180

When JIT is enabled, Array.from is better on almost browser engine without ChackraCore(MSEdge).(Is call high cost?)

However, Micro-benchmarking is Hard.

If need to more details, We should collection real performance test like #1528
( #1528 inject console log to hls.js and collect the results)

📝 Notes:

We can create a util function like copyArray for removing unnessary if in main code.

const copyArray = (() => {
    if (typeof Array.from === 'function') {
        return (arrayLike) => {
            return Array.from(arrayLike);
        }
    } else {
        return (arrayLike) => {
            return Array.prototype.slice.call(arrayLike);
        }
    }
})();

johnBartos · 2018-05-20T03:36:17Z

Seems like slice is faster considering the % usages of browsers using Hlsjs (we see a large majority using Chrome). I personally like using Array.prototype.slice.call over adding more code. If we want to use rest params we should inspect the transpiled output to make sure it's not doing anything suboptimal. Polyfills can have nasty surprises.

Korilakkuma · 2018-05-20T05:45:39Z

If Array.from is better than Array.prototype.slice.call, I would like you consider this PR.

If we want to use rest params we should inspect the transpiled output to make sure it's not doing anything suboptimal. Polyfills can have nasty surprises.

I think so too.

tchakabam · 2018-05-23T09:28:00Z

Two questions for everyone discussing here:

How many milliseconds do we gain in our real use-case here, i.e how often does this call run per second in average?
For those who are against native feature detection and then using the best option, why are you against it? Performance? This feature detection code he put there runs exactly once upon loading the library. It will not take more than 1ms in the first place I guess :)

tchakabam · 2018-05-23T09:33:55Z

@Korilakkuma btw the "easier to read" argument isn't that convincing, since you could alias the whole slice thing anyway (which is what you did).

The real thing that is bugging me with this code is, why can't we call someArray.slice() directly on this? This should be an array from the start in the first place. And why does this need a copy anyway? Is it only to make something into an array that isn't really? Can someone explain that?

Korilakkuma · 2018-05-24T13:00:52Z

Array.prototype.slice and Array.from create new array by shallow copy. Therefore, if the results are the same regardless of which one use, I think that it is better to use one with better performance.

tchakabam · 2018-05-25T10:49:31Z

@Korilakkuma In principle I agree with you. Doing the feature-detection once is probably worth the potential performance gain anyway. About my previous comment: I was basically questioning a bit the argumentative overhead vs the actual gain we have here from this change :)

However, I appreciate a lot that this proposition of yours has brought up a different issue in my opinion around this part of the code:

The issue I see is that it is unclear why we are copying this array in the first place.

I think a much greater improvement may be to not have to do a copy at all :)

It seems we are doing this because that object is at first initialized as a plain Object instance. Would we create it as Array from the start, we wouldn't have to do this copying? Or is there another reason why we need to do a copy? I couldn't really imagine. However I didn't take time to look at this in depth.

Korilakkuma · 2018-05-28T12:27:30Z

There is a question.

It seems we are doing this because that object is at first initialized as a plain Object instance.

It seems to be initialized as undefined or TypedArray. Is this my mistaken ?

https://github.com/video-dev/hls.js/blob/master/src/remux/mp4-generator.js#L329

tchakabam · 2018-06-01T14:56:36Z

@Korilakkuma We are talking about the data property of the tracks thing here: https://github.com/video-dev/hls.js/blob/master/src/remux/mp4-generator.js#L333

That is what we call Array.prototype.slice.call(data) on. The question I asked is: Why is this necessary? The tracks object could be created with an array in the first place.

The line you linked is unrelated to the problem.

Korilakkuma · 2018-06-01T15:19:58Z

@tchakabam

I see. Do you think the following code ?

// ...

for (i = 0; i < track.sps.length; i++) {
  data = track.sps[i];
  len = data.byteLength;
  sps.push((len >>> 8) & 0xFF);
  sps.push((len & 0xFF));
  sps = sps.concat(data); // Unnecessary `Array.prototype.slice.call` or `Array.from`
}

// ...

tchakabam · 2018-06-04T13:57:15Z

Well, yes that's what I am saying.

I guess however this code you just posted will break because data is not an Array, so you can't call concat on it (which is why we convert it to an Array currently with prototype.slice).

If I'd want to fix it, I'd try and look wehre the track instance is initialized, and make sure that sps property is created as an instance of Array :) 👍

Korilakkuma · 2018-06-04T15:19:21Z

I think that sps property is created as an instance of Array by the following code ...

https://github.com/video-dev/hls.js/blob/master/src/demux/tsdemuxer.js#L712

(But, I'm not sure ...)

tchakabam · 2018-06-04T20:51:52Z

that is track.sps, but our data is one element of that array. which should again be an array.

this is really confusing :) I guess it's about finding out what unit.data for a type is here.

If I am not wrong this would actually be a Uint8Array.

That explains it now. We are actually converting that to a plain Array, because that's what it is in the mp4-generator. Hm, that's pretty bad :/ MP4-generator should use Uint8Array as well. But at least now we know why :D

So seems this is the go-to way to convert Uint8Array to Array. Btw people out there claim one should use Array.from where it is available. I also would assume that is more optimized than the slice "hack". So let's please do this feature detection on module decl :) 👍

tchakabam · 2018-06-04T20:54:14Z

Btw this is a great example how confusing code can be that lacks types, and how they just make things more clearly fit together.

Korilakkuma · 2018-06-05T05:13:34Z

So let's please do this feature detection on module decl

Is that the following code ?

class MP4 {
  static copyAsArray = typeof Array.from === 'function' ? Array.from : Array.prototype.slice.call;

  /* ... */

  static avc1 (track) {
    let sps = [], pps = [], i, data, len;
    // assemble the SPSs

    for (i = 0; i < track.sps.length; i++) {
      data = track.sps[i];
      len = data.byteLength;
      sps.push((len >>> 8) & 0xFF);
      sps.push((len & 0xFF));
      sps = sps.concat(MP4.copyAsArray(data)); // SPS
    }

    // assemble the PPSs
    for (i = 0; i < track.pps.length; i++) {
      data = track.pps[i];
      len = data.byteLength;
      pps.push((len >>> 8) & 0xFF);
      pps.push((len & 0xFF));
      pps = pps.concat(MP4.copyAsArray(data));
    }

    /* ... */

  }

  /* ... */
}

tchakabam · 2018-06-05T11:51:56Z

hey. what i meant by "on module declaration" is exactly what is the current diff of this PR :) No need for any changes from my side!

I don't think it makes any sense to do this as a static method of the class.

But, if you'd like to do it really nicely, I would put this into it's own module in utils. This is something very general purpose.

Also, it would be great if you could check wether this kind of Uint8Array to Array conversion is being done elswhere in the code and then re-use the same approach consistently.

Finally, I would not call it copy... anymore since that is not the purpose. Let's have a utils module with an exported function called makeArrayFromUint8Array for example :)

tchakabam

almost there!

tchakabam · 2018-06-05T12:56:21Z

src/utils/make-array-from-array-like.js

@@ -0,0 +1,5 @@
+'use strict';
+
+const MakeArrayFromArrayLike = typeof Array.from === 'function' ? Array.from : Array.prototype.slice.call;


lower case this, since it is not a constructor: makeArrayFromArrayLike

tchakabam · 2018-06-05T12:56:57Z

src/utils/make-array-from-array-like.js

+
+const MakeArrayFromArrayLike = typeof Array.from === 'function' ? Array.from : Array.prototype.slice.call;
+
+export default MakeArrayFromArrayLike;


export this named, like export makeArrayFromArrayLike

tchakabam · 2018-06-05T12:57:34Z

src/remux/mp4-generator.js

@@ -3,6 +3,7 @@
 */

 // import Hex from '../utils/hex';
+import makeArrayFromArrayLike from '../utils/make-array-from-array-like';


import this named: import { makeArrayFromArrayLike } from '../utils/make-array-from-array-like';

:) 👍

Korilakkuma · 2018-06-05T13:07:32Z

@tchakabam

I see. So I made the following changes.

Implement function in utils directory
Change copyAsArray to makeArrayFromArrayLike

tchakabam · 2018-06-05T13:30:04Z

src/utils/make-array-from-array-like.js

@@ -0,0 +1,3 @@
+'use strict';


you don't need 'use strict' here, please remove that.

we are compiling all this stuff with babel and bundling it with webpack, these tools take care of this stuff.

Removed Array.prototype.slice.call

88056fc

- Use rest parameters - Use `Array.from`

Korilakkuma changed the title ~~Removed Array.prototype.slice.call~~ Array.prototype.slice.call May 19, 2018

Korilakkuma changed the title ~~Array.prototype.slice.call~~ Use Array.from instead of Array.prototype.slice.call May 19, 2018

Revert rest parameters

3e91c4e

Use wrapper function

2306a46

Implement utility function for making an instance of Array

e131eda

tchakabam suggested changes Jun 5, 2018

View reviewed changes

Change to named exports

85e217c

tchakabam suggested changes Jun 5, 2018

View reviewed changes

Remove 'use strict'

a64e67d

tchakabam approved these changes Jun 5, 2018

View reviewed changes

tchakabam merged commit 897c4fc into video-dev:master Jun 5, 2018

Korilakkuma deleted the feature/remove-array-prototype-slice-call branch June 5, 2018 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `Array.from` instead of `Array.prototype.slice.call` #1722

Use `Array.from` instead of `Array.prototype.slice.call` #1722

Korilakkuma commented May 19, 2018 •

edited

johnBartos commented May 19, 2018

Korilakkuma commented May 19, 2018 •

edited

azu commented May 19, 2018 •

edited

azu commented May 19, 2018 •

edited

Korilakkuma commented May 19, 2018

johnBartos commented May 19, 2018

Korilakkuma commented May 19, 2018 •

edited

Korilakkuma commented May 19, 2018

azu commented May 20, 2018 •

edited

SpiderMonkey

Chakra

V8 --harmony

V8

johnBartos commented May 20, 2018 •

edited

Korilakkuma commented May 20, 2018

tchakabam commented May 23, 2018 •

edited

tchakabam commented May 23, 2018 •

edited

Korilakkuma commented May 24, 2018

tchakabam commented May 25, 2018

Korilakkuma commented May 28, 2018

tchakabam commented Jun 1, 2018 •

edited

Korilakkuma commented Jun 1, 2018

tchakabam commented Jun 4, 2018 •

edited

Korilakkuma commented Jun 4, 2018

tchakabam commented Jun 4, 2018 •

edited

tchakabam commented Jun 4, 2018

Korilakkuma commented Jun 5, 2018

tchakabam commented Jun 5, 2018 •

edited

tchakabam left a comment

tchakabam Jun 5, 2018

tchakabam Jun 5, 2018

tchakabam Jun 5, 2018

Korilakkuma commented Jun 5, 2018

tchakabam Jun 5, 2018

		@@ -0,0 +1,5 @@
		'use strict';

		const MakeArrayFromArrayLike = typeof Array.from === 'function' ? Array.from : Array.prototype.slice.call;


		const MakeArrayFromArrayLike = typeof Array.from === 'function' ? Array.from : Array.prototype.slice.call;

		export default MakeArrayFromArrayLike;

Use Array.from instead of Array.prototype.slice.call #1722

Use Array.from instead of Array.prototype.slice.call #1722

Conversation

Korilakkuma commented May 19, 2018 • edited

This PR will...

Why is this Pull Request needed?

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

johnBartos commented May 19, 2018

Korilakkuma commented May 19, 2018 • edited

azu commented May 19, 2018 • edited

azu commented May 19, 2018 • edited

Korilakkuma commented May 19, 2018

johnBartos commented May 19, 2018

Korilakkuma commented May 19, 2018 • edited

Korilakkuma commented May 19, 2018

azu commented May 20, 2018 • edited

SpiderMonkey

Chakra

V8 --harmony

V8

johnBartos commented May 20, 2018 • edited

Korilakkuma commented May 20, 2018

tchakabam commented May 23, 2018 • edited

tchakabam commented May 23, 2018 • edited

Korilakkuma commented May 24, 2018

tchakabam commented May 25, 2018

Korilakkuma commented May 28, 2018

tchakabam commented Jun 1, 2018 • edited

Korilakkuma commented Jun 1, 2018

tchakabam commented Jun 4, 2018 • edited

Korilakkuma commented Jun 4, 2018

tchakabam commented Jun 4, 2018 • edited

tchakabam commented Jun 4, 2018

Korilakkuma commented Jun 5, 2018

tchakabam commented Jun 5, 2018 • edited

tchakabam left a comment

Choose a reason for hiding this comment

tchakabam Jun 5, 2018

Choose a reason for hiding this comment

tchakabam Jun 5, 2018

Choose a reason for hiding this comment

tchakabam Jun 5, 2018

Choose a reason for hiding this comment

Korilakkuma commented Jun 5, 2018

tchakabam Jun 5, 2018

Choose a reason for hiding this comment

Use `Array.from` instead of `Array.prototype.slice.call` #1722

Use `Array.from` instead of `Array.prototype.slice.call` #1722

Korilakkuma commented May 19, 2018 •

edited

Korilakkuma commented May 19, 2018 •

edited

azu commented May 19, 2018 •

edited

azu commented May 19, 2018 •

edited

Korilakkuma commented May 19, 2018 •

edited

azu commented May 20, 2018 •

edited

johnBartos commented May 20, 2018 •

edited

tchakabam commented May 23, 2018 •

edited

tchakabam commented May 23, 2018 •

edited

tchakabam commented Jun 1, 2018 •

edited

tchakabam commented Jun 4, 2018 •

edited

tchakabam commented Jun 4, 2018 •

edited

tchakabam commented Jun 5, 2018 •

edited