Fix auto-scheduler profile mismatch #2421
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When using the auto-scheduler on certain inputs, the auto-scheduler may throw an error stating: "Error: profile used for auto-scheduling doesn't match the provided program." This was reported in Issue souffle-lang#2346 by another user, providing a small example to test on. Running a `git bisect` using the example reveals that the commit 43a4ce3 (Working refactor of SIP Graph, 2022-06-23) was the first to introduce the error. Inspecting the profiler output for that commit and the parent commit, I noted that some new indices appear the join columns and they should probably not be there. After deep inspection of the refactor, I've noticed the following diff potentially changes the program behaviour: src/ast/analysis/JoinSize.cpp in 43a4ce3 (Working refactor of SIP Graph, 2022-06-23) ```diff - auto& dependentVars = varToOtherVars.at(var->getName()); - if (!dependentVars.empty() && - std::includes(providedVars.begin(), providedVars.end(), - dependentVars.begin(), dependentVars.end())) { - joinColumns.push_back(argIdx); - ++numBound; - continue; ``` src/ast/utility/SipGraph.cpp in 55c3500 (Working SIPGraph for SelingerSIPS, 2022-06-23) ```diff + if (std::includes(groundedVars.begin(), groundedVars.end(), dependentVars.begin(), + dependentVars.end())) { + boundColumns.insert(argIdx); + } ``` This commit adds the empty check, and this change makes the error disappear. While I am not certain about whether this is the correct fix, the use of `std::includes` on an empty range could be a potential cause of bugs. I tested the join column output against the parent commit of the 43a4ce3 (Working refactor of SIP Graph, 2022-06-23), and can confirm that the join columns in the profile output are the same. Closes souffle-lang#2346
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2421 +/- ##
==========================================
+ Coverage 77.75% 77.89% +0.13%
==========================================
Files 474 479 +5
Lines 31164 31398 +234
==========================================
+ Hits 24232 24456 +224
- Misses 6932 6942 +10
|
|
As a side note, this fix does not cover all issues when using the auto-scheduler. I have encountered another profile mismatch error after the fix, and unfortunately I wasn't able to pin down the root cause. |
quentin
approved these changes
Jul 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using the auto-scheduler on certain inputs, the auto-scheduler may throw an error stating:
"Error: profile used for auto-scheduling doesn't match the provided program."
This was reported in Issue #2346 by another user, providing a small example to test on.
Running a
git bisectusing the example reveals that the commit 43a4ce3 (Working refactor of SIP Graph, 2022-06-23) was the first to introduce the error.Inspecting the profiler output for that commit and the parent commit, I noted that some new indices appear the join columns and they should probably not be there.
After deep inspection of the refactor, I've noticed the following diff potentially changes the program behaviour:
src/ast/analysis/JoinSize.cpp
in 43a4ce3 (Working refactor of SIP Graph, 2022-06-23)
src/ast/utility/SipGraph.cpp
in 55c3500 (Working SIPGraph for SelingerSIPS, 2022-06-23)
This commit adds the empty check, and this change makes the error disappear. While I am not certain about whether this is the correct fix, the use of
std::includeson an empty range could be a potential cause of bugs.I tested the join column output against the parent commit of the 43a4ce3 (Working refactor of SIP Graph, 2022-06-23), and can confirm that the join columns in the profile output are the same.
Closes #2346