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

perf: replace glob with fast-glob #2280

Merged
merged 3 commits into from
Feb 5, 2024

Conversation

abejfehr
Copy link
Contributor

@abejfehr abejfehr commented Jan 27, 2024

Summary:

Closes #2271

I was investigating how pod install was so slow in React Native, and it seems like a lot of time is spent in IO#gets:

image

Upon further investigation, this is actually Ruby waiting for the output of node .../node_modules/@react-native-community/cli/build/bin.js config.

When investigating what's slow about node .../node_modules/@react-native-community/cli/build/bin.js config, I noticed that a lot of time was spent in globs (about ~17 seconds on an M1 Mac):

image

In my testing, I've observed that glob can be replaced with fast-glob to make performance considerably better with the same output, so I'd like to propose that glob be replaced with fast-glob in this project.

Test Plan:

The unit tests are great, but one thing I'm encountering is that the FS mock doesn't seem to play well with something that fast-glob is doing internally.

Even though it works in practice, it doesn't seem to work in tests 😢 it's like fs.__setMockFilesystem isn't taking effect or something (it is once, but never subsequently. It's like fast-glob doesn't re-scan the disk? idk)

Any help here would be greatly appreciated.

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@abejfehr abejfehr changed the title Replace glob with fast-glob perf: replace glob with fast-glob Jan 27, 2024
@liamjones
Copy link
Contributor

The unit tests are great, but one thing I'm encountering is that the FS mock doesn't seem to play well with something that fast-glob is doing internally.

Even though it works in practice, it doesn't seem to work in tests 😢 it's like fs.__setMockFilesystem isn't taking effect or something (it is once, but never subsequently. It's like fast-glob doesn't re-scan the disk? idk)

Any help here would be greatly appreciated.

There was a previous PR for this which went stale which mentioned that same issue - there might be a solution in there already? #2016

@szymonrybczak
Copy link
Collaborator

hey @abejfehr! thanks for contribution, could you please rebase on top of main, I've deleted update-metro.js file which is no longer anymore used. Also as @liamjones said you can take a look at my attempt on replacing glob with fast-glob - where I fixed units tests. I see that Windows E2E test is failing there, logs expired so I can't check if this was related, but anyway it may be worth checking 👍

@abejfehr
Copy link
Contributor Author

abejfehr commented Jan 30, 2024

hey @abejfehr! thanks for contribution, could you please rebase on top of main, I've deleted update-metro.js file which is no longer anymore used. Also as @liamjones said you can take a look at my attempt on replacing glob with fast-glob - where I fixed units tests. I see that Windows E2E test is failing there, logs expired so I can't check if this was related, but anyway it may be worth checking 👍

Thanks! I've rebased.

Looking at the prior art helped me resolve the FS mocking issue and it passes tests now locally, so I think it's ready for a review

@szymonrybczak
Copy link
Collaborator

@abejfehr thanks! looks like Windows E2E is failing now, same as in my PR 🙈 Could you please look at this? I'll also try to search for solution for this.

@abejfehr
Copy link
Contributor Author

abejfehr commented Feb 3, 2024

I ran the tests on a Windows machine and noticed that it wasn't building properly, and it looks like that's because I replaced glob with fast-glob in linkPackages.js and build.ts as well, but there it doesn't do unixifyPaths and it wouldn't be available yet for use there anyways.

I opted for adding glob/@types/glob as devDependencies for the whole workspace instead and using them just for the build/link script.

Now on Windows for me there's ~3 failing tests but they seem to be due to very specific paths and the ones in main fail for the same reason too, so I'm hoping they pass in CI 🤞

Copy link
Collaborator

@szymonrybczak szymonrybczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @abejfehr! 🙏 CI Green ✅

@thymikee thymikee merged commit 9bfee86 into react-native-community:main Feb 5, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants