Closed PhoebeSzmucer closed 1 month ago
Name | Link |
---|---|
Latest commit | a85ac260d3c1587fc5acd0e128a3691d5f940fe2 |
Latest deploy log | https://app.netlify.com/sites/salsa-rs/deploys/669eaf3fc07f2c00085609da |
Hmm. Ideally we would accumulate in execution order, I think. A queue won't really do it, that will give us a breadth-first search. I think what we want to do is to push the children onto the stack but in reverse order -- something like this. But you also have to remove the hashset insertion I think and instead do the insertion when you pop off the stack. Otherwise this case would print wrong:
A {
B {
D
}
C,
D,
}
it would print like A, B, C, D instead of A, B, D, C.
Or you could go back to a recursive function (I wouldn't bother with the generic method, I'd just pull out a simple helper function). I think that would be fine, especially since the recursion depth is reflecting an execution that already took place. =)
@nikomatsakis is the failure a flake?
Yeah, the miri failure is #520
I was thinking @PhoebeSzmucer that it'd be good to add a test for one of the more complex scenarios -- e.g. to test duplicates. Are you up to add something like that?
@nikomatsakis yes if be happy to add more tests.
I think there are accumulator tests that indirectly test for the lack of duplicates already (the accumulator-dag one, accumulator-execution-order one that I added here), but I can add some more.
@nikomatsakis I added one more complex test here https://github.com/salsa-rs/salsa/pull/524/commits/a85ac260d3c1587fc5acd0e128a3691d5f940fe2
@PhoebeSzmucer nice! Approved.
I noticed that some of the accumulated values were getting dropped, and I traced it to the
accumulated_by
function only accumulating from direct children.Let me know if this fix is any good - I'm quite new to Rust and this codebase, and I wasn't sure how to best implement this recursion.