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

Es6.array.from and Es6.array.iterator do something with Float32Array/Array? which makes it 100x slower on Chrome #377

Closed
vlajos opened this issue Feb 10, 2018 · 15 comments

Comments

@vlajos
Copy link

vlajos commented Feb 10, 2018

Demo scripts:
http://lavela.hu/bug/test-array.from.html
http://lavela.hu/bug/test-array.iterator.html
http://lavela.hu/bug/test-array.others.html

Speed results on my machine:

  • test-array.from.html 1236.399999999776 ms
  • test-array.iterator.html 1345.0000000002547 ms
  • test-array.others.html 11.300000000119326 ms

Other notes:

  • The third one is just a reference that the other ES6.array.* modules does not seem to cause any trouble. (The speed is the same without core-js.)
  • It seems that the bug does something with the chrome tab which affects the later executions in the same tab. (Run first a slow one and then try to run the array.others one. It will be also slow.)
  • The test script: http://lavela.hu/bug/test.js But practically the new Float32Array(arr) operation is getting slower.
  • es6.promise seems to affected as well. (I guess through a shared internal dependency.)
  • Firefox does not seem to be affected.
  • My chrome version: Version 64.0.3282.119 (Developer Build) built on Debian buster/sid, running on Debian buster/sid (64-bit) (But I had the same results on 63.x on Windows.)
  • Core.js version: 2.5.3
  • All the test details are here: http://lavela.hu/bug.tar.gz (built es6 modules, script to build them, test htmls, test js, test results, flame graphs, performance profiles.)

Related flamegraphs from chrome:
test-array from perf
test-array iterator perf
test-array others perf

(The story started originally here: pixijs/pixijs#4662 and here: babel/babel#7362 But I think I copied everything relevant here.)

@vlajos vlajos changed the title Es6.array.from and Es6.array.iterator do something with Float32Array which makes it 100x slower on chrome Es6.array.from and Es6.array.iterator do something with Float32Array/Array? which makes it 100x slower on chrome Feb 13, 2018
@vlajos vlajos changed the title Es6.array.from and Es6.array.iterator do something with Float32Array/Array? which makes it 100x slower on chrome Es6.array.from and Es6.array.iterator do something with Float32Array/Array? which makes it 100x slower on Chrome Feb 13, 2018
@vlajos
Copy link
Author

vlajos commented Feb 22, 2018

Can I help anything with this issue?

@zloirock
Copy link
Owner

zloirock commented Mar 6, 2018

@vlajos now I have no time for exploring this issue. Feel free explore it and propose changes.

@vlajos
Copy link
Author

vlajos commented Mar 10, 2018

@zloirock I tried to dig a bit deeper.
It seems that this line is the problematic one:
https://github.com/zloirock/core-js/blob/master/modules/_iter-detect.js#L18
At least if I extract the same logic, it seems that this script can cause the same trouble:

<script>
var ITERATOR = Symbol.iterator;
var arr = [7];
var iter = arr[ITERATOR]();
iter.next = function () { return { done: safe = true }; };
arr[ITERATOR] = function () { return iter; };
</script>
<script src="test.js"></script>

And if I comment out that line, the problem disappears. (Or if I return after/before that line in the original function.)
Unfortunately I am a little lost what is happening here...

I tried to revert arr[ITERATOR] to the original in a later step, but it did not make any difference.
Any idea?

A little shorter example:

<script>
var arr = [7];
arr[Symbol.iterator] = function () {};
</script>
<script src="test.js"></script>

(I'll carry on, just wanted to share these results first...)

@zloirock
Copy link
Owner

Funny. Seems somehow any array with a custom iterator somehow deoptimise array iteration (which also used in typed array constructors) in V8.

@vlajos
Copy link
Author

vlajos commented Mar 11, 2018

@zloirock
Would it worth reporting the v8 devs?
What does this iter_detect tries to achieve? Is not it possible to do it without that assignment somehow?

@zloirock
Copy link
Owner

zloirock commented Mar 11, 2018

You could report it.

It's detection of support iterables in methods / constructors which should accept it.

It's possible to do it without that assignment, but IIRC that caused some problems in some old Promise.{all, race} implementations which accept only arrays and throws uncatchable errors to dev tools.

@zloirock
Copy link
Owner

I added a workaround to v3 branch. Feel free to test it. If it will cause some problems, I will revert it.

@vlajos
Copy link
Author

vlajos commented Mar 11, 2018

@zloirock Thanks! I have just tried it, but there is no difference.
I checked out the v3 branch, then npm i;npm run build
I used this test page in chrome:

<script src="core.js"></script>
<script src="test.js"></script>

Just do double check if the new code is applied:

$ sed '2q;d' core.js 
 * core-js 3.0.0-alpha.1
grep -c ITERATION_SUPPORT core.js 
3

But the speed of the test is still the same.
So it seems that it still deoptimises that thing. :-(

@zloirock zloirock reopened this Mar 11, 2018
@zloirock
Copy link
Owner

So seems the problem not (or not only) in this code.

@zloirock
Copy link
Owner

I reworked typed arrays and iterators polyfills, found some cases which can theoretically somehow deoptimise it and now, after calling your test, seems it's fixed in v3 branch. If it still not fixed, please, report me.

@vlajos
Copy link
Author

vlajos commented Mar 25, 2018

@zloirock Thank you very much. The test is definitly quick now. Do you think I could test our "full" app against the v3 branch? Is it stable enough?

@zloirock
Copy link
Owner

@vlajos yes, now it's enough stable, today I finished testing on all supported engines. Seems actual version of v3 branch will be published as the first public alpha version of core-js@3 without any serious changes.

@zloirock
Copy link
Owner

[email protected] is available.

@vlajos
Copy link
Author

vlajos commented Mar 27, 2018

@zloirock Thanks. I checked earlier and i can confirm that our app works properly with [email protected].
I see there are some more related commits, should I retest it with them?

@zloirock
Copy link
Owner

@vlajos nope, it's duplicates after rebasing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants