search

file-icons / atom

Atom file-specific icons for improved visual grepping.
MIT License
1.32k stars 250 forks source link

For-of transform + Minify #804

Closed aminya closed 4 years ago

aminya commented 4 years ago
Alhadis commented 4 years ago

Have you actually benchmarked this?

aminya commented 4 years ago

I can say for-of transform has a huge effect: https://github.com/aminya/TypeScript-optimization#summary

Alhadis commented 4 years ago

I ask again: have you actually benchmarked this? Because I'd be surprised if it made any perceivable difference in loading time.

I politely remind you that I'm no fan of micro-optimising, especially when it involves adding pointless complexity to a project.

aminya commented 4 years ago

I ask again: have you actually benchmarked this? Because I'd be surprised if it made any perceivable difference in loading time.

I politely remind you that I'm no fan of micro-optimising, especially when it involves adding pointless complexity to a project.

I have benchmarked this, but the loading time is very hard to measure.

However, I can say the runtime is enhanced by using traditional for loops (for-of transform babel plugin). See https://github.com/aminya/TypeScript-optimization#summary

For example, this loop uses for-of: https://github.com/file-icons/atom/blob/ac7ada7ff82b3b21ed7ccdb1110d77f7386cf6ae/lib/icons/icon-tables.js#L134

Alhadis commented 4 years ago

Pros:

Cons:

Alhadis commented 4 years ago

Also, for-of isn't a performance killer. It used to be, back when V8 was still using Crankshaft (its old optimising compiler, created in the pre-ES6 era). There's still an added performance overhead compared with C-style for loops, but inline caching makes this a non-issue once the code gets hot and it tiers up through Turbofan (V8's new/current optimising compiler).

Moreover, a lot of this is still obviated by memoisation.

aminya commented 4 years ago

Pros:

* Negligible performance improvements

Since I don't know the code, I don't know how to benchmark the exact function which is hot, but the overal benchmark shows improvements.

Also, for-of isn't a performance killer. It used to be, back when V8 was still using Crankshaft (its old optimising compiler, created in the pre-ES6 era). There's still an added performance overhead compared with C-style for loops, but inline caching makes this a non-issue once the code gets hot and it tiers up through Turbofan (V8's new/current optimising compiler).

Moreover, a lot of this is still obviated by memoisation.

This isn't true yet. https://github.com/aminya/TypeScript-optimization#summary I have done the tests myself in the latest Node. for-of is 30% to 100% slower. I have tested both ES5 and ES6. ES5 doesn't have a for-of syntax at all.

aminya commented 4 years ago 
* One can no longer `require("file-icons/lib/…/singleton.js");` in the dev-console to debug/tinker the actual runtime instances.

* Either larger package size (caused by including two different copies of the source-code and/or a source-map), or undebuggable code if a source-map isn't included in the package.

These can be fixed very easily. By using a script that builds the code before publishing. All the typescript projects use a similar approach.

aminya commented 4 years ago

If you don't want this PR, I am going to make another PR manually optimizing the loops.

Alhadis commented 4 years ago

@aminya Make sure to include benchmarks proving there's a noticeable increase in speed.

aminya commented 4 years ago

@aminya Make sure to include benchmarks proving there's a noticeable increase in speed.

I can use the dev console + performance tab for benchmarking, however, as I mentioned before, I have done serious benchmarks for all different types of arrays (strings, numbers), objects, etc. https://github.com/aminya/TypeScript-optimization#summary

However, if you are reluctant to merge it anyways, please let me know before I spend my time fixing these.

Alhadis commented 4 years ago

Since I plan to rewrite most of the package's code anyway, I'd say it's not worth the time and effort.

aminya commented 4 years ago

Since I plan to rewrite most of the package's code anyway, I'd say it's not worth the time and effort.

Thanks for letting me know. If you wanna rewrite this, I suggest using a static language that is compiled to Wasm:

This package doesn't do much dom manipulation, and so it can really benefit from doing this.

Alhadis commented 4 years ago

The performance-critical parts will be C, compiled to WebAssembly. The remainder will be plain JavaScript.