search

file-icons / atom

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

For of #805

Closed aminya closed 4 years ago

aminya commented 4 years ago

Optimizes some of the for-of loops.

aminya commented 4 years ago

The rationale behind this PR is described here: https://github.com/aminya/TypeScript-optimization#summary

Measuring the performance is hard in such a project that is tied to the Atom, but we can see some improvements here and there (5-20ms improvement for each loop) by measuring things in the performance tab in dev mode:

Before:

MatchName, MatchInterpreter

2020-03-28 05_06_42-Project — C__Users_yahyaaba_Documents_GitHub_atom-file-icons_examples — Atom Nig

MatchIcon:

2020-03-28 04_49_03-

Path-Strtegy, Icon-tables 2020-03-28 04_33_56-Microsoft Edge

After:

MatchName, MatchInterpreter ~(20-30ms)

2020-03-28 05_08_49-Project — C__Users_yahyaaba_Documents_GitHub_atom-file-icons_examples — Atom Nig

MatchIcon ~(10-15ms)

2020-03-28 04_47_59-Microsoft Edge

Path-Strtegy (~10ms), Icon-tables (~5ms) 2020-03-28 04_35_03-Microsoft Edge

aminya commented 4 years ago

Your Eslint plugin is broken, and it doesn't load. I fixed some of your rules manually. I recommend using an eslintrc instead of using a package.

Alhadis commented 4 years ago

Your Eslint plugin is broken

Could you be more specific? What exactly was the error?

I fixed some of your rules manually.

I don't know what you changed, but the PR is now littered with diff-noise. Take a look.

aminya commented 4 years ago

Your Eslint plugin is broken

Could you be more specific? What exactly was the error?

Can't copy image

aminya commented 4 years ago

I don't know what you changed, but the PR is now littered with diff-noise. Take a look.

These are trailing spaces (usually hidden) that are removed automatically on save. image

You can remove them all using: https://github.com/aminya/AcuteGit#remove-trailing-whitespace

Alhadis commented 4 years ago

First, the ESLint error is being caused by a reference to @typescript-eslint/eslint-plugin, which this package doesn't even use. Going by the screenshot, it's being referenced from ..\.eslintrc, which I assume is located in the directory containing your fork.

These are trailing spaces tabs (usually hidden) that are removed.

Yes, I know. I'd appreciate it if you could revert that.

aminya commented 4 years ago

First, the ESLint error is being caused by a reference to @typescript-eslint/eslint-plugin, which this package doesn't even use. Going by the screenshot, it's being referenced from ..\.eslintrc, which I assume is located in the directory containing your fork.

That was strange. It was in another directory, but was being picked by the eslint-plugin

aminya commented 4 years ago

I reverted the trailing space issue because of diff noise. Fix the CI yourself.

Alhadis commented 4 years ago

That was strange. It was in another directory, but was being picked by the eslint-plugin

Yeah, that's long been a source of confusion for ESLint users. That's why personal config files are getting deprecated in the next major version.

Alhadis commented 4 years ago

Thanks. Not sure what's causing the latest CI error, but it's (probably) unrelated to this PR. I'll look into it.

Thanks for your patience.

aminya commented 4 years ago

You're welcome.

I left some other loops that I haven't benchmarked yet (like for ([a,b] of c)).

Alhadis commented 4 years ago

Silly me, I neglected to check that each of the iterables in question was really an Array. The LinguistStrategy loop you modified had been iterating through a Set, hence the broken tests. I've reverted that particular change.