-
-
Notifications
You must be signed in to change notification settings - Fork 355
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
Fix resultBuilt(dirty mechanism) in hls-graph #4238
Fix resultBuilt(dirty mechanism) in hls-graph #4238
Conversation
deps <- readIORef deps | ||
let changed = if runChanged == ChangedRecomputeDiff then built else maybe built resultChanged result | ||
built' = if runChanged /= ChangedNothing then built else changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set the new build time to changed
might update the build time to a older one
@@ -187,7 +187,6 @@ instance NFData RunMode where rnf x = x `seq` () | |||
-- | How the output of a rule has changed. | |||
data RunChanged | |||
= ChangedNothing -- ^ Nothing has changed. | |||
| ChangedStore -- ^ The stored value has changed, but in a way that should be considered identical (used rarely). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it is ever used or would be used, but we could add back if there is a need for this.
Do we have any tests for this mechanism? |
this seems to make sense. how did you discover this? |
Actually, I've discovered the Yep, I'll see if some test could be added. |
…dirty-mechanism-of-hls-graph' into 4237-clarify-the-dirty-mechanism-of-hls-graph
I've added two test, one from the lower level and one from higher level to demonstrate the bug and ensure no regression. |
I'd like @wz1000 to check the tests make sense just to be sure! |
nag @wz1000 |
Let's merge it if no one object to this. I've been using this branch and no regression yet. And the bench reuslt look pretty promising |
Fix #4237
resultBuilt
might updated to an older value, which make a rule result that should be clean back to dirty.It might trigger un-needed recomputation. Fixing it could minimize the recomputation further.