indutny / fft.js

The fastest JS Radix-4/Radix-2 FFT implementation
278 stars 31 forks source link

ES5 version #6

Closed drom closed 7 years ago

drom commented 7 years ago

I see that library uses const, let, Array.fill constructs that makes it non ES5 compatible. Is any performance (other) reasons not to use just plain ES5? In our application we are still running ES5.1 compatible iojs-1.2.1 run-time.

indutny commented 7 years ago

There are some sanity reasons to not use it :) const provides checks that help in writing safer code, let scoping is awesome too.

I'd suggest using babel if possible.

danigb commented 7 years ago

In my own experience with dsp-kit the let and const constructs added a big performance penalty over the standard var (I suppose is not optimized yet). Maybe it's a good idea to add some benchmarks about.

indutny commented 7 years ago

@danigb there are few notes in the source code where I use var instead of let/const to bump the speed, but they should not be needed in the future anymore.

The performance is almost maximized at this point, so I'm absolutely sure that no remaining let/const uses lowers it.

drom commented 7 years ago

I understand the babel story of const and let. In this particular case I am fine with single line sed script.

indutny commented 7 years ago

@kgryte any reason for that smile? 😉

kgryte commented 7 years ago

@indutny :) Anything that makes me use out-of-band tooling or special flags just to run is something I am highly likely to avoid, irrespective of how ace the coding is.

Personally, the purported problems addressed by using const and let I have yet to see as issues. While AOT information can be advantageous to compilers, most modern JavaScript compilers are able to infer constant values (as seen in, e.g., LICM). Lexical scoping, while allowing you to possibly use fewer variables, does not provide much material advantage, and is generally unnecessary if using small focused functions.

Lastly, using ES6 APIs like Array#fill seem wholly unnecessary when a 5-line function, which does not need to handle generic array-like objects, suffices. My experience has been that, global APIs, such as fill, indexof, etc, are generally slower and not worth the convenience, especially when such APIs are either buggy (indexof) or not supported in various environments (e.g., fill in IE and Opera).

Quite possibly I am just old and stodgy.

indutny commented 7 years ago

@kgryte I see what you mean, and in fact I wasn't supportive for using the newest features until recently.

From the little experience that I got from working with ES6, const and let already caught lots of potential problems for me. Additionally, it shifts the paradigm a bit and lets me think differently about the programs (particularly, about how the data flows through it), which I find nice.

If you feel strongly about it - I'm happy with reversing this in the library. There just few places where these features are used, and it should be ok to be ES5 here. However, I'd like to clearly understand that struggles that you may have with using/deploying this library at the moment. If the environment is node - it should be mostly fine, unless you use some antique versions. If the env is browser - you very likely already babel-ize the code in which case use of const/let is not a big deal.

Please tell me more 👍

danigb commented 7 years ago

@indutny Great! I really like the code you have right now.

@kgryte I like const because I tend to think that it is more useful for humans than computers. Makes easier to understand the code, and catch one kind of bug (accidentally change a const value) faster.

indutny commented 7 years ago

@danigb I like the scope-visibility of both let/const too. Prevents "leaks" of values from the loops into surrounding code.

drom commented 7 years ago

@indutny I appreciate all scope-visibility of let and const and I agree that in some cases they give you extra elegant code. I also agree with @kgryte the impact is minimal if you code composed of small focused functions. On other hand I sort of afraid to run babel over somebody`s performance optimized library, knowing what transpiler can produce less optimal code.

kgryte commented 7 years ago

@drom Actually that is a good point. Running a transpiler is a good way to get unoptimized code.

drom commented 7 years ago

@indutny about my use case. I use you library compare accuracy with my code https://github.com/drom/fourier and I test against node 0.10, 0,12, 1,2,3,4,5,6,7 as well as browsers FF, CH, Edge. An it broke my TravisCI (that is how I know about it ;) https://travis-ci.org/drom/fourier/builds/210839044

indutny commented 7 years ago

Yikes, I see. I can't blame for using iojs, but you can't blame me for not supporting it either :wink:

How does precision of the libraries compare?

indutny commented 7 years ago

As a compromise, I've pushed the version without fill(0) call :) At least no polyfills are required at this point.

The performance of babel generated code in dist/ is identical to the original ES6 code, so I wouldn't worry much about it for a moment.

drom commented 7 years ago

@indutny thank you for fill(0) fix. It enables me to run on iojs-1. This is one of platforms that I have to support. Because our desktop version of the product has to run on the CentOS 6, so we use custom compiled NWJS 0.12.2, that is based of iojs v1.2.1.

drom commented 7 years ago

@indutny the precision (introduced noise) is in par with regular f64 DFT implementation is under -150dB.

indutny commented 7 years ago

Nice!

indutny commented 7 years ago

Tentatively closing this.