jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.52k stars 263 forks source link

don't use arguments #142

Closed shahen94 closed 6 years ago

shahen94 commented 6 years ago

Accessing arguments inside a function is slow so. Now used args instead of arguments

jayphelps commented 6 years ago

Thanks for the PR! Can you clarify what you mean? My understanding is that just referencing arguments hasn’t been slow in many years?

Also—this code doesn’t do the same thing as before, and now breaks. args and arguments actually refer to arguments from different functions. args was poorly named in this case, which likely caused the confusion.

shahen94 commented 6 years ago

@jayphelps Yes - I just understand it why my changes broke functionality.

Accessing to arguments is slow for V8 engine and for Chakra Core many years and now as well. So using spread operator should interpreted faster than access to arguments.

http://www.jspatterns.com/arguments-considered-harmful/

However, it's not a good idea to use arguments for the reasons of:

performance security The arguments object is not automatically created every time the function is called, the JavaScript engine will only create it on-demand, if it's used. And that creation is not free in terms of performance. The difference between using arguments vs. not using it could be anywhere between 1.5 times to 4 times slower, depending on the browser

Edited: I just run some benchmarks and can't see any huge difference between using arguments vs rest operator.

I'll fix this functionality and will stay it to you - to decide merge this or not.

Thanks!

jayphelps commented 6 years ago

@shahen94 ahhh got it, I misunderstood what this code was doing. I thought it was doing handle(...arguments) which definitely doesn't have any notable performance issues (it's just sugar for handle.apply(undefined, arguments), but since we're literally passing the variable as-is handle(arguments) that's a different story and could prevent it from being JIT'able because the function you pass it to could actually modify the calling function's arguments. Nice catch! 🏅

I'm guessing the reason why in practice this didn't have any difference in performance is that the handle function is likely inlined and the compiler can tell that no one ever touches anything on the arguments object except arguments.length which is safe. Either way not all browsers could be smart enough to detect that or we might later change the code and cause a deopt. So I think this PR is 💯

Thanks again!