Skip to content

Conversation

hborla
Copy link
Member

@hborla hborla commented Aug 11, 2020

The missing /*updateWorkList=*/false argument turns mergeEquivalenceClasses into a very expensive operation because it needs to gather constraints in order to update the worklist. It isn't necessary to activate these constraints anyway because the conforms to ExpressibleBy* constraints will be unsolvable at this point.

Resolves: rdar://problem/66807959

@hborla
Copy link
Member Author

hborla commented Aug 11, 2020

@swift-ci please smoke test

@hborla hborla requested a review from xedin August 11, 2020 14:52
@hamishknight
Copy link
Contributor

Should we remove the default argument for updateWorkList? It looks like there's only 2 callers relying on it, with the majority passing false.

@hborla
Copy link
Member Author

hborla commented Aug 11, 2020

@hamishknight Yeah, I think that's a good idea. That certainly would have prevented me from making this mistake 😅

ConstraintSystem::mergeEquivalenceClasses.

Most callers don't use the default, and it's important to consider
the value of this argument for each call to mergeEquivalenceClasses.
@hborla
Copy link
Member Author

hborla commented Aug 11, 2020

@swift-ci please smoke test

@hborla hborla merged commit c4541c5 into swiftlang:master Aug 11, 2020
@hborla hborla deleted the join-perf-regression branch August 11, 2020 20:58
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