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

Improve performance of resolving manifest file paths #5871

Merged

Conversation

anlaital-oura
Copy link
Collaborator

@anlaital-oura anlaital-oura commented Feb 2, 2024

Short description πŸ“

Running tuist edit in a large git repository is quite slow at the moment. Running Time Profiler indicates that globing is the root cause of the slowness as it globs the entire repository. This is despite .tuistignore being present, as exclusion is done only after globing.

These changes reduce the time it takes to run tuist edit in our large repository from 31s to 3s 1.5s 0.27s in release mode (M2 Max). Related discussion on Tuist Slack.

I tried running mise run tuist:test but it fails acceptance tests even on a clean checkout. Could you advise on how tests should be run locally? I don't know if it's because of Tuist 4 changes, but a lot of the documentation seems a bit outdated on this.

Before:

image

After:

image

How to test the changes locally 🧐

Run tuist scheme with edit argument against a custom working directory that is a large git repository.

Contributor checklist βœ…

  • The code has been linted using run mise run lint:fix
  • The change is tested via unit testing or acceptance testing, or both
  • The title of the PR is formulated in a way that is usable as a changelog entry
  • In case the PR introduces changes that affect users, the documentation has been updated

Reviewer checklist βœ…

  • The code architecture and patterns are consistent with the rest of the codebase
  • Reviewer has checked that, if needed, the PR includes the label changelog:added, changelog:fixed, or changelog:changed, and the title is usable as a changelog entry

@fortmarek
Copy link
Member

I tried running mise run tuist:test but it fails acceptance tests even on a clean checkout. Could you advise on how tests should be run locally? I don't know if it's because of Tuist 4 changes, but a lot of the documentation seems a bit outdated on this.

Yeah, currently, you need to run the acceptance tests using one of the acceptance tests scheme TuistAcceptanceTests, TuistTestAcceptanceTests, etc. The Tuist-Workspace scheme currently doesn't have the necessary env variables. This is missing in the docs right now, we'll make sure to add it soon πŸ™

@anlaital-oura
Copy link
Collaborator Author

I will convert this to draft until I have time to fix the acceptance test issues now that I know how to run them locally.

@anlaital-oura anlaital-oura marked this pull request as draft February 2, 2024 09:46
@anlaital-oura
Copy link
Collaborator Author

Acceptance tests now pass locally with the exception of test_ios_app_with_transitive_framework() failing that also fails with clean main. I had to make the implementation just ever-so-slightly more complex, but hopefully now it supports all use cases. Let me know if some special testing is needed for this outside of automated tests and running this against our own project!

@anlaital-oura anlaital-oura marked this pull request as ready for review February 2, 2024 10:33
let absolutePath = AbsolutePath(stringLiteral: path)
if hasValidManifestContent(absolutePath) {
tuistManifestsFilePaths.append(absolutePath)
if candidatePath.hasSuffix(".swift") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: you can use a guard and continue to reduce intentation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

This is an amazing improvement πŸ‘

@fortmarek fortmarek added the changelog:changed PR will be listed in the Changed section of CHANGELOG label Feb 3, 2024
@fortmarek fortmarek merged commit 86c38a8 into tuist:main Feb 3, 2024
7 of 8 checks passed
@anlaital-oura anlaital-oura deleted the anlaital/improve-tuist-edit-performance branch February 3, 2024 09:28
Copy link
Contributor

@pepicrft pepicrft left a comment

Choose a reason for hiding this comment

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

just wow. Thanks a lot for this @anlaital-oura

@pepicrft
Copy link
Contributor

pepicrft commented Feb 5, 2024

@all-contributors add @anlaital-oura for code

Copy link
Contributor

@pepicrft

I've put up a pull request to add @anlaital-oura! πŸŽ‰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:changed PR will be listed in the Changed section of CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants