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 DescriptionPackage.resolveBuildProducts() performance #134

Merged

Conversation

Ryu0118
Copy link
Contributor

@Ryu0118 Ryu0118 commented Jul 7, 2024

Motivation

In my personal application, the package contains a large number of targets, which causes the resolveBuildProducts() function to take a significant amount of time to execute. The common libraries added as dependencies to multiple targets lead to redundant generation of the same BuildProducts, resulting in overhead.

Proposed Solution

To address this issue, I created a BuildProductsResolver class. This class caches the targets passed to the buildProducts(from:) function in a visitedTargets property. By doing this, when the same target is encountered again, it returns early, preventing redundant processing. This change significantly reduces execution time by eliminating repeated operations.

Copy link
Owner

@giginet giginet left a comment

Choose a reason for hiding this comment

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

This basic concept seems to be fine. However, we're supporting It for Swift 6 snapshot.
These implementations may break when SwiftPM was updated.

Merge this PR before #133, or could you follow its changes after merging that PR?

Sources/ScipioKit/DescriptionPackage.swift Outdated Show resolved Hide resolved
Sources/ScipioKit/DescriptionPackage.swift Outdated Show resolved Hide resolved
@Ryu0118
Copy link
Contributor Author

Ryu0118 commented Jul 8, 2024

OK, I will implement Swift 6 support on this branch after #133 is merged.

@giginet
Copy link
Owner

giginet commented Jul 9, 2024

I merged #133. Could you fix this PR for Xcode 15.4 and Xcode 16?

@Ryu0118 Ryu0118 force-pushed the improve-resolve-build-products-performance branch from 8e28a73 to 999ef2e Compare July 10, 2024 04:28
@Ryu0118 Ryu0118 force-pushed the improve-resolve-build-products-performance branch from 999ef2e to 1bb3d13 Compare July 10, 2024 04:37
@Ryu0118
Copy link
Contributor Author

Ryu0118 commented Jul 10, 2024

@giginet OK, I have fixed it. Could you review these changes again?

@Ryu0118 Ryu0118 requested a review from giginet July 10, 2024 05:09
Copy link
Collaborator

@freddi-kit freddi-kit left a comment

Choose a reason for hiding this comment

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

implementation is reasonable for me 👍
@Ryu0118 Could you show me the result of this refactoring? How times it speed up?

@Ryu0118
Copy link
Contributor Author

Ryu0118 commented Jul 10, 2024

@freddi-kit I tested the execution of resolveBuildProducts() in my OSS project, and the time was reduced from 2.4 seconds in the previous implementation to 0.006 seconds, resulting in an approximately 392 times speed improvement.

@giginet
Copy link
Owner

giginet commented Jul 10, 2024

Sorry, the current main seems to be broken. I fixed this on #135. Please moment.

@Ryu0118 Ryu0118 requested a review from freddi-kit July 11, 2024 03:30
@Ryu0118 Ryu0118 force-pushed the improve-resolve-build-products-performance branch from b378923 to 6219b25 Compare July 11, 2024 03:37
@Ryu0118 Ryu0118 force-pushed the improve-resolve-build-products-performance branch from 6219b25 to 03a4e7c Compare July 11, 2024 03:37
Copy link
Owner

@giginet giginet left a comment

Choose a reason for hiding this comment

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

Great change! It makes sense to use memorization.

I can't wait to measure the impact of our product.
Thank you 😄

@giginet giginet merged commit 948007b into giginet:main Jul 12, 2024
4 checks passed
@Ryu0118 Ryu0118 deleted the improve-resolve-build-products-performance branch July 12, 2024 15:24
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.

3 participants