file-icons / atom

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

Fix performance hole #814

Closed aminya closed 4 years ago

aminya commented 4 years ago

This part of the code takes 300ms for its execution! This is not what people want in the production code. You can measure it by

const t1 = window.performance.now()
// code ...
console.log(window.performance.now()-t1)

Before

image image

After

image image

Alhadis commented 4 years ago

Thanks for watching so closely. Unfortunately, you misinterpreted what's meant by "tests". I meant literal test-files in a user's project that contain interpreter directives:

Figure 1

See daa9906 and #812 for background and rationale.

aminya commented 4 years ago

300 ms is a lot. My whole Atom loads in 400ms, and this is taking about that time! This is so hot, that it has destroyed all the file-icons goodness.

If this is just a feature: it should be disabled by default and add an atom.config to let the user enable it, if they want to. It can even be a separate package.

Another solution could be hardcoding these similar to #815, or do this operation offline and load them from a file in the disk (similar to .iconsdb). Remember that JS/JSON parser of V8 is written in C++ and is super fast, and in contrast, searching through the whole icon set is very expensive and slow.

Alhadis commented 4 years ago

Remember that JS/JSON parser of V8 is written in C++ and is super fast, and in contrast, searching through the whole icon set is very expensive and slow.

You really need to read up on what you're talking about... 😜 Ignition (V8's bytecode interpreter) and TurboFan are more complex than what you're describing. It's not a straight lex-and-parse at whatsoever.

aminya commented 4 years ago

On master, this is fixed now. Making the map offline was the best move.