search

cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

Workaround v8 optimizer bug #404

Closed briancavalier closed 9 years ago

briancavalier commented 9 years ago

After some testing by @anodynos, @spion, and myself, we found a few things other than adding an empty try/finally in _beget that seem to workaround the v8 bug. One is to do away with the arguments + ternary operator deref in then(), in favor of reinstating the onProgress parameter for now.

The other is to split _beget and delegate its work to a helper function.

After making that change, I realized that there's no need for a similar arguments+ternary check in the main when() function, if we reinstate the parameter there as well. So, I changed that one too.

Close #403 Close #345

briancavalier commented 9 years ago

Using the test in #403, this runs to completion (7000 iterations) on my macbook pro (osx 10.10) using both node 0.10.33 and 0.11.14.

I'll setup tests to run overnight tonight to build a little more confidence.

unscriptable commented 9 years ago

Why not add all three deoptimization changes? Seems like the safest thing to do unless you know for sure that future versions of node won't upgrade to a version of V8 that learns to optimize one or more of these deoptimizations?

anodynos commented 9 years ago

+1 for safety @unscriptable See what happened here https://github.com/cujojs/when/issues/403#issuecomment-62818946

spion commented 9 years ago

My guess about arguments was based on it being a known source of trouble for the v8 optimizer. Removing its use should make the code look fairly typical and therefore much less likely to hit a bug in the JIT (hopefully).

Ultimately I have no idea whats causing it, except that it requires indexing the arguments object inside a ternary operator to reproduce. @petkaantonov might be interested to take a look though (yep thats a ping :P)

briancavalier commented 9 years ago

Why not add all three deoptimization changes

We don't know that adding all of them will matter or won't matter in some hypothetical future v8 optimization issue. All we really know is that any one of the changes we've tried seems to help.

That's why I'm inclined to do the simplest thing that can possibly work: pick one. Doing away with arguments is actually a nice change regardless of whether it fixed this issue or not, so I think it makes sense to pick that as the one.

That said, I'm ok with making _beget delegate to another function. That's a clean change. I'm not ok with adding an empty try/finally because it's gross :D and not something we'd do otherwise. If nothing else worked, then I'd say let's add it.

briancavalier commented 9 years ago

I just force-pushed an update that also includes the _beget delegation.