[Sema] Use different solution vectors for ComponentSteps created by DependentComponentSplitterStep
#37964
Conversation
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test macOS |
|
Build failed |
|
Build failed |
ahoppen
force-pushed
the
pr/solutions-scoring
branch
from
3aaa97f to
59f9d43
Compare
|
@swift-ci Please test |
|
Build failed |
| @@ -294,6 +294,11 @@ class DependentComponentSplitterStep final : public SolverStep { | |||
| /// Array containing all of the partial solutions for the parent split. | |||
| MutableArrayRef<SmallVector<Solution, 4>> AllPartialSolutions; | |||
|
|
|||
| /// The solutions computed the \c ComponentSteps created for each partial | |||
| /// solution combinations. Will be merged into the final \c Solutions vector | |||
lib/Sema/CSStep.h
Outdated
| /// The solutions computed the \c ComponentSteps created for each partial | ||
| /// solution combinations. Will be merged into the final \c Solutions vector | ||
| /// in \c resume. | ||
| SmallVector<std::unique_ptr<SmallVector<Solution, 2>>, 2> ContextualSolutions; |
There was a problem hiding this comment.
Should we consider using plain std::vector here instead of wasting stack?
There was a problem hiding this comment.
I don’t have strong opinions but I think based on the fact that all but two tests passed with the SmallVector of size 2, it’s reasonable to assume that there are often only ComponentSteps that need to be taken here, so I think it’s worth to avoid the heap allocation of std::vector here, especially since the member of ContextualSolutions is only a pointer and thus not really occupying a whole lot of stack space.
There was a problem hiding this comment.
We just might not have too many examples of complex result builders in the suite., but I am okay with leaving it as small vector for now too.
There was a problem hiding this comment.
Hmm, that’s true. I still think there’s still value in using the SmallVector and the overhead should be relatively low.
There was a problem hiding this comment.
The only concern I have is for sufficiently complex result builders not to run out of stack in memory constrained environments because each dependent component would now allocate this storage and keep it along the whole duration of sub-path processing in splitter step.
There was a problem hiding this comment.
Maybe I’m missing something but I thought the constraint solver had a very low stack usage because it is using a work list to schedule new steps.
But if you think that’s an issue, I’ll change it to std::vector. As I said, I don’t have strong feelings about this.
Edit: I changed it to std::vector for now.
There was a problem hiding this comment.
Constraint system and solutions use plenty of stack, we had to convert some of the small vectors to std::vector to mitigate that.
|
I would be great if you could add a unit test test with two dependencies that produce multiple solutions and non-empty original score to make sure that score behaves correctly. Could be done separately. |
… `DependentComponentSplitterStep` Currently all `ComponentSteps` created by `DependentComponentSplitterStep` share the same `Solutions` vector. Because of this, the `ComponentStep`s might modify solutions created by previous `ComponentStep`s. Use different `Solutions` vectors for each `ComponentStep` to avoid sharing information between the `ComponentStep`s. The concrete manifestation in the added test case is that the `Bar` overload gets added to `Solutions`, it’s score gets reduced by its `ComponentStep` original score, then the `Foo` overload gets added to `Solutions` and both solutions have their score decreased by the `OriginalScore` of `Foo`’s `ComponentStep`, causing `Bar`’s score to underflow. Fixes rdar://78780840 [SR-14692]
ahoppen
force-pushed
the
pr/solutions-scoring
branch
from
59f9d43 to
c3ce40d
Compare
I changed the code completion test case to a Sema test case that tests the same behavior. Could you take a look at it? |
|
@swift-ci Please test |
|
Build failed |
I think that's good. I'll look into adding a more specific unit test which would validate scores directly, it would be pretty hard to do with integration tests using swift code. |
|
@swift-ci Please test macOS |
|
@swift-ci Please test Windows |
|
Build failed |
|
@swift-ci Please test macOS |
|
Build failed |
|
@swift-ci Please smoke test macOS |
Currently all
ComponentStepscreated byDependentComponentSplitterStepshare the sameSolutionsvector. Because of this, theComponentSteps might modify solutions created by previousComponentSteps. Use differentSolutionsvectors for eachComponentStepto avoid sharing information between theComponentSteps.The concrete manifestation in the added test case is that the
Baroverload gets added toSolutions, it’s score gets reduced by itsComponentSteporiginal score, then theFoooverload gets added toSolutionsand both solutions have their score decreased by theOriginalScoreofFoo’sComponentStep, causingBar’s score to underflow.Fixes rdar://78780840 [SR-14692]