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

Added example to Unsupported phi use of const or let variable #10

Closed
wants to merge 1 commit into from

Conversation

billhance
Copy link

I came across the "Unsupported phi use of const or let variable" bug while learning es6 and practicing a selection sort. I noticed my implementation was way slower than I expected so this might be a misunderstanding of some block scope gotcha in JS on my part.

Here is a fiddle that demonstrating the bug: https://jsfiddle.net/billhance/wfz9fcs6/1/

In console profiler, I saw an increase of 800% where input n = 1000 when this bug was present.

image

image

image

@leeoniya
Copy link

leeoniya commented Dec 9, 2016

I've run into this before also and noticed the issue went away after transpiling down to ES5. i wonder if any of the recent ES6 improvements [1] cover this case.

[1] http://v8project.blogspot.com/2016/12/v8-release-56.html

@billhance
Copy link
Author

@leeoniya I think you are right that is specific to ES6 because when I run the ES5 equivalent (specifically let to var) the bug goes away. Unfortunately, I don't have a beta build handy to test the new v8 changes.

@vhf
Copy link
Owner

vhf commented Dec 9, 2016

You might want to try the snippet triggering this bailout on node nightly or on the latest version of v8. Both are available on https://mono.morph.ist

I'm happy to try it for you if you prefer.

@billhance
Copy link
Author

Thanks for the suggestion @vhf, I've never used that tool before.
Not sure if I ran the test correctly, It seems like it's not fixed in the node nightly. https://mono.morph.ist/job/9johJCT9QWuC8NAbK

@vhf
Copy link
Owner

vhf commented Dec 9, 2016

Thanks for the feedback @billhance . Yeah this tool is kinda new, I released it pretty recently and I know there are a few bugs in there. I'll work on them when I have time.

The results of your tests are quite surprising. TurboFan doesn't bail out on node 6.

I ran a similar test here https://mono.morph.ist/job/X7yqLKNJqTukaxu3g but it bails out on all known node versions it seems.

Also interesting is that using the destructuring variable swap trick doesn't bail out on nightly but bails out on node 6 : https://mono.morph.ist/job/pdCkANEcwDcLxEysD (see line 15)

@billhance
Copy link
Author

Very cool tool 👍 . So am I to assume correctly that this is a recently introduced bug in v8 (sorry, a bit out of my depth)? If so, would https://bugs.chromium.org/p/v8/issues be the appropriate place to file the bug report? Thanks.

@vhf
Copy link
Owner

vhf commented Dec 10, 2016

Well, it's really not a bug. The results you got depend on several things.

For instance, if transpiling to ES5 "solves" the issue, it doesn't mean it's a bug in later versions. The semantics are just different, and rightfully handled differently. The scoping of let and const have no ES5 equivalent. The whole TDZ (temporal dead zone) concept was introduced by ES2015 and seems not to be optimizable by Crankshaft. It also seems to me this bailout still happens in Turbofan under very specific conditions, I just cannot get which ones.

Also worth keeping in minde, when we observe some node versions optimizing this snippet via turbofan and some later node versions not optimizing it, it might simply mean that the later versions don't have turbofan enabled.

I don't work on V8 so I cannot really say whether opening an issue there would be the right move. I'm pretty sure it wouldn't be closed without someone giving some kind of explanation though. :)

@billhance
Copy link
Author

I'm sure that team has enough on their plate for now to spend time indulging my curiosity so I'll hold off. Thanks so much for helping me understand this stuff better @vhf.

@dmonad
Copy link

dmonad commented May 1, 2018

This is already fixed in current Firefox and Chrome.

@vhf
Copy link
Owner

vhf commented May 1, 2018

@dmonad Firefox never used v8 and Chrome does not use Crankshaft anymore. This repo serves as documentation even though it's legacy by now. ;)

@dmonad
Copy link

dmonad commented May 1, 2018

Ah okay. Thanks for the clarification!

@vhf vhf closed this Jul 13, 2018
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.

4 participants