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

optimize repository updates #6685

Merged
merged 3 commits into from Jul 12, 2023
Merged

Conversation

tomerd
Copy link
Member

@tomerd tomerd commented Jul 8, 2023

motivation: improve performance

changes:

  • instead of binary update flag, use a strategy that takes into account the version
  • implement the update strategy for source control based dependency
  • update call sites and tests

@@ -79,7 +82,7 @@ final class ContainerProvider {
// Otherwise, fetch the container from the provider
self.underlying.getContainer(
for: package,
skipUpdate: self.skipUpdate,
updateStrategy: self.skipUpdate ? .never : .always, // FIXME
Copy link
Member Author

@tomerd tomerd Jul 10, 2023

Choose a reason for hiding this comment

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

it could be nice optimization if we could use ifNeeded in PubGrub too, but I am not sure it would be correct. @neonichu ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I think we probably need something more elaborate there. If there's an existing pin, I believe we try to stick to it in PubGrub, but we would need the ability to switch the strategy if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

left this for follow up PR

Copy link
Member

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

LGTM, but missing doc comments for new types

@tomerd tomerd self-assigned this Jul 11, 2023
@MaxDesiatov
Copy link
Member

While there were minor updates to the tests, I wonder if there's a way to expose different strategies and test that those also work as expected?

@neonichu
Copy link
Member

Might also be interesting to look into checkoutRepository use in Workspace, it seems like that could also do unnecessary fetches and even re-copying of checkouts.

@tomerd
Copy link
Member Author

tomerd commented Jul 11, 2023

While there were minor updates to the tests, I wonder if there's a way to expose different strategies and test that those also work as expected?

yes that is still TODO

Might also be interesting to look into checkoutRepository use in Workspace, it seems like that could also do unnecessary fetches and even re-copying of checkouts.

yes, but in follow up PR

motivation: improve performance

changes:
* instead of binary update flag, use a strategy that takes into account the version
* implement the update strategy for source control based dependency
* update all call sites and tests
@tomerd
Copy link
Member Author

tomerd commented Jul 12, 2023

@swift-ci smoke test

@tomerd
Copy link
Member Author

tomerd commented Jul 12, 2023

this should be ready for final review

@@ -703,7 +703,8 @@ public final class GitRepository: Repository, WorkingCheckout {
/// Returns true if a revision exists.
public func exists(revision: Revision) -> Bool {
self.lock.withLock {
(try? callGit("rev-parse", "--verify", revision.identifier)) != nil
let output = try? callGit("rev-parse", "--verify", "\(revision.identifier)^{commit}")
Copy link
Member Author

Choose a reason for hiding this comment

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

👀 this was not actually working as intended but was never a problem I guess

@tomerd
Copy link
Member Author

tomerd commented Jul 12, 2023

@swift-ci smoke test

@tomerd
Copy link
Member Author

tomerd commented Jul 12, 2023

@swift-ci smoke test windows

@tomerd tomerd merged commit 106c681 into apple:main Jul 12, 2023
5 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.

None yet

3 participants