Comment by dsp1234 | Hacker News Reader

Comment by dsp1234 | Hacker News Reader

Parent Full thread

Optimization decisions that V8 team have made are much more universal than it might seem.

That's certainly possible, but maybe a competing VM can do better, but finds that everyone has coded specific V8 optimizations. They may be hesitant to push out their implementation. It may be a better optimization, but it causes existing code to run slower on their VM. Thus back to the original statement that implementation quirks find their way into developer code.

for (IWantThisVariableToEndUpOnGlobalObject in obj) { }

The variable has to be in the local scope and can't be in any higher or lower scope, not just a global scope (which your words acknowledge, but your code snippet doesn't). It's easy to end up sending the prop variable through a closure accidentally. Some real world examples[0][1][2]. Note that the solution requires pushing out to another function just to get around the deopt. With the sproutcore example being very clear, as they made the change specifically to satisfy the V8 VM.

[0] - https://github.com/paperjs/paper.js/issues/466

[1] - http://www.html5rocks.com/en/tutorials/performance/mystery/

[2] - https://github.com/sproutcore/sproutcore/blob/master/CHANGEL... (search "Removes V8 "ForIn is not fast case" warning")

> The variable has to be in the local scope and can't be in any higher or lower scope, not just a global scope (which your words acknowledge, but your code snippet doesn't)

Sure! I am just trying to say that in my opinion whenever you have a non-local variable used as iteration variable in for-in then you most probably have a bug in your code. I tried to illustrate this with an global variable example because it's a common source of JS bugs - when people leak things into a global namespace by accident.

> Some real world examples[0][1][2]

These links refer to a different bailout reason --- "ForIn is not fast case". The original ForIn support in Crankshaft (written coincidentally by me) only supported this kind of for-in because it was the important case to support and the one where you can get good performance with reasonable investment of time.

Given time and bug reports from people hitting this bailout I would certainly extend ForIn support to cover a more generic case (assuming that supporting more generic case would make some code faster), however just in a couple of months after I landed this initial support I switched to a different project, so I never had chance to revisit this.

This bailout reason is actually not in V8 anymore - as now V8 supports both fast and slow cases in Crankshaft[1]

[1] https://github.com/v8/v8/blob/master/src/crankshaft/hydrogen...

Hey, maybe you're the right person to ask - why does `arguments` so easily deoptimize in every major engine (well, V8 and SpiderMonkey, at least)?

It seems that engines could easily detect the most common munging of `arguments` (such as [].slice.call(arguments), Array.prototype.slice.call(arguments). Array.from(arguments) and the like) and allow those to be optimized. Doing so would speed up a very large amount of code.

Do you have any insight on why that still has not been done after so many years?

> Do you have any insight on why that still has not been done after so many years?

I can't really speak for either V8 or SpiderMonkey but I think there are few reasons -

a) nobody got to doing it, even though it was discussed multiple times, e.g. for V8 it just was not the right time to implement it as its trying to completely revamp its optimization pipeline and certain Crankshaft idiosyncrasies make this sort of optimization pretty brittle;

b) I am not entirely sure that it will actually speedup that much code, this kind of code is rarely on an extremely hot code-path (extremely hot code paths must strive to avoid allocation entirely!);

c) there is a reasonable workaround that provides good performance (manual loop);

d) ES6 provides something better than arguments object: rest arguments.

Thanks for that.

I'll give you an example of where it hits hard - Event Emitters. Some of the largest EE libs in the NodeJS ecosystem still munge the arguments object to pass args to listeners. I've sent PRs to some of them, but the deopt caused by the arguments munging seems to slow down the whole function and everything it calls, which can be quite significant (like an entire render loop).

ES6 solves this problem nicely but it will be a long time before we can deploy it natively. Thankfully, Babel handles it correctly and uses a proper for loop. So the need to fix this is less urgent than ever.

As a super late followup:

This really cements the case that the poster above was stating.

no edge-case of an implementation can make its way into programmers' habits, something that tends to happen a lot with JavaScript.

There is code in the wild right now that fixes (perceived or actual) problems in the code's interaction with the V8 engine. And worse, that developer code no longer solves any problem.

whenever you have a non-local variable used as iteration variable in for-in then you most probably have a bug in your code.

Agreed, I'm pretty sure every linter would pick that up anyways. That's not really the cases that I saw though, mostly it was stuff where the key is needed in a function/closure (just threw this together straight in the text area here):

  function Intercept(obj1, obj2){
     for(var key in obj1){
       obj2[key] = function(){
           console.log('called: ', key);
           obj1[key]();
       }
     }
  }

Note that in the document I referenced, this was under section 5.1. I'm not sure if you would consider them being the same or not, but that's where it's listed in the document, so that's I how I cited it.

> no edge-case of an implementation can make its way into programmers' habits, something that tends to happen a lot with JavaScript.

Edge cases should not, and they almost never do... However performance optimizations is a very special area - you have to know how things are implemented and utilize this knowledge.

> mostly it was stuff where the key is needed in a function/closure (just threw this together straight in the text area here):

This code is also buggy - all closures will point to the same `key`.

Reply on news.ycombinator.com