rtsao / csjs

:sparkles: Modular, scoped CSS with ES6
MIT License
576 stars 32 forks source link

Fix deopt caused by slicing arguments object #23

Closed STRML closed 8 years ago

STRML commented 8 years ago

Gotta go fast

rtsao commented 8 years ago

Awesome, thank you for pointing this out, TIL. :smile:

By the way, I used IRHydra2 to look up the intermediate representations of your proposed change and the following (which omits the length cache):

var values = Array(arguments.length - 1);
for (var i = 1; i < arguments.length; i++) {
  values[i - 1] = arguments[i];
}

Your suggestion and the above yielded identical intermediate representations, which seems to confirm what is discussed in this post: http://mrale.ph/blog/2014/12/24/array-length-caching.html

All else being equal, I think would prefer omitting the length cache, if you wouldn't mind updating your PR.

Regardless, I'd definitely like to start benchmarking (https://github.com/rtsao/csjs/issues/12) and hopefully identify further optimizations.

STRML commented 8 years ago

Glad you're thinking about that. I was too. I found an old thread from about 2013 saying that preallocating the array doesn't affect performance, but in my testing and many others', it clearly does. It appears that the length access is indeed properly cached in an optimized function. V8 is a fickle yet powerful beast.

I've amended the commit.

STRML commented 8 years ago

By the way, thanks for pointing out IRHydra2. That is seriously, seriously awesome. Been looking for something like this to validate some of my assumptions/guesses.