Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster atom.packages.getAvailablePackages #20899

Merged
merged 5 commits into from Jul 10, 2020

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Jun 7, 2020

Description of the Change

  • Removes the unnecessary directory existence check. This path is read from fs.readdir, and so we are sure that it exists!

  • Increasing the performance of getting the list of all available atom packages

Quantitative Performance Benefits

For my setup (having 209 packages), it speeds up the command by ~10ms.

Possible Drawbacks

Nothing I can think of

Verification Process

I measured it using this method:

const t1 = window.performance.now()
const packages = atom.packages.getAvailablePackages()
console.log(window.performance.now() - t1)

// to check the result
console.log(packages)

Also, by using this change, it decreased the loading time of my package package-switch by using this custom command which implements the change:
https://github.com/aminya/package-switch/blob/003894af9eb31bb279721bb102788194078da086/src/bundles.ts#L90

Applicable Issues

Release Notes

  • Increasing the performance of getting the list of all available atom packages

@aminya
Copy link
Contributor Author

aminya commented Jun 7, 2020

The macOS single test failure is totally unrelated to this PR. It is for package-find-and-replace

@UziTech
Copy link
Contributor

UziTech commented Jun 7, 2020

I believe that check is for whether it is a directory or file not whether the directory exists.

I'm guessing it will fail if there is a file in the packages directory without that check.

@aminya
Copy link
Contributor Author

aminya commented Jun 8, 2020

@UziTech
In this commit, I used the native withFileTypes in readdir to filter out non-folders. This is both fast and reliable.

@aminya aminya force-pushed the getAvailablePackages branch 2 times, most recently from f29b7da to 728b720 Compare Jun 8, 2020
@lkashef
Copy link
Contributor

lkashef commented Jun 12, 2020

Hey @aminya, thanks for the contribution 🙇‍♂️

Please check the CI failure or even better run script/lint locally.

@darangi darangi added the triaged label Jul 3, 2020
src/package-manager.js Outdated Show resolved Hide resolved
src/package-manager.js Outdated Show resolved Hide resolved
Co-authored-by: Sadick <sadickjunior@gmail.com>
@lkashef lkashef merged commit 4a9d56f into atom:master Jul 10, 2020
1 check passed
@tada369

This comment has been minimized.

@DeeDeeG

This comment has been minimized.

Copy link

@edimaliusman edimaliusman left a comment

Give mehh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants