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

Fix resultBuilt(dirty mechanism) in hls-graph #4238

Merged
merged 19 commits into from
Jun 3, 2024

Conversation

soulomoon
Copy link
Collaborator

@soulomoon soulomoon commented May 17, 2024

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.

@soulomoon soulomoon linked an issue May 17, 2024 that may be closed by this pull request
deps <- readIORef deps
let changed = if runChanged == ChangedRecomputeDiff then built else maybe built resultChanged result
built' = if runChanged /= ChangedNothing then built else changed
Copy link
Collaborator Author

@soulomoon soulomoon May 17, 2024

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).
Copy link
Collaborator Author

@soulomoon soulomoon May 17, 2024

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.

@soulomoon soulomoon marked this pull request as ready for review May 17, 2024 11:09
@michaelpj michaelpj requested a review from wz1000 May 17, 2024 11:41
@michaelpj
Copy link
Collaborator

Do we have any tests for this mechanism? hls-graph is reasonably stand-alone, I would hope we could write a test for this?

@wz1000
Copy link
Collaborator

wz1000 commented May 17, 2024

this seems to make sense. how did you discover this?

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 17, 2024

Actually, I've discovered the Changed and Built are somewhat off when fixing the dritiness hiding problem, but I forget about it at the time since it it won't hide the dirtiness but instead increase the chance of it.
I am reviewing the code and it comes back to me.

Yep, I'll see if some test could be added.

@soulomoon
Copy link
Collaborator Author

soulomoon commented May 17, 2024

I've added two test, one from the lower level and one from higher level to demonstrate the bug and ensure no regression.

@michaelpj
Copy link
Collaborator

I'd like @wz1000 to check the tests make sense just to be sure!

@michaelpj
Copy link
Collaborator

nag @wz1000

@soulomoon soulomoon added the performance Issues about memory consumption, responsiveness, etc. label Jun 1, 2024
@soulomoon
Copy link
Collaborator Author

soulomoon commented Jun 3, 2024

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

@michaelpj michaelpj enabled auto-merge (squash) June 3, 2024 14:18
@michaelpj michaelpj merged commit 322ac35 into master Jun 3, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: hls-graph performance Issues about memory consumption, responsiveness, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buggy dirty mechanism in hls-graph
3 participants