react: Update only dependents of the just-set cell #131
Conversation
There was a problem hiding this comment.
Reviewers: Obviously, it wouldn't be necessary to accept this PR, since the example solution already passes the tests. The only reason this is here is that in #126 (comment) we wondered whether it was possible to only update cells that would need an update. I agreed to propose this after I completed my submission to the website.
| struct child *next = child->next; | ||
| if (c == child->cell->input1) { | ||
| // Don't double-free for a compute2 cell. |
There was a problem hiding this comment.
That was my main concern with making this. I thought it would be very tricky to free every cell exactly once, but every compute cell has exactly one input1, so it works out.
There was a problem hiding this comment.
I don't really understand the need for this if. Can you explain this for me?
There was a problem hiding this comment.
let's say it's not there, so that we always call destroy_cell(child->cell).
In that case, even this:
struct cell *input1 = create_input_cell(r, 1);
struct cell *input2 = create_input_cell(r, 2);
struct cell *output = create_compute2_cell(r, input1, input2, plus);will double-free:
destroy_reactor calls destroy_cell(input1)
destroy_cell(input1) calls destroy_cell(output)
destroy_reactor calls destroy_cell(input2)
destroy_cell(input2) calls destroy_cell(output)
Everything else is correct; it just needed to avoid a second call to destroy_cell(output)
There was a problem hiding this comment.
Ahh, that makes much more sense. Thanks!
| return c; | ||
| } | ||
|
|
||
| #define each_child(c) struct child *child = (c)->child; child; child = child->next |
There was a problem hiding this comment.
too obscure? I didn't like repeating myself.
| } | ||
| free(child->cell); | ||
| struct cb *cb = c->cb; | ||
| while (cb) { |
There was a problem hiding this comment.
my submission on the website asks whether it is possible to reduce the duplication between the three while loops
|
Looks good to me outside of a bit of confusion in |
|
I like that you're updating this 👍, but if you're going to do that I'd suggest also at least adding NULL checks... At some point I may bring back up the problem, but after I actually write a solution. (I've spent the last 20 minutes just attempting to get it to compile an empty skeleton. The API has 9 functions and 2 structs that need to be defined, and all of their inputs need to be used (to avoid the compiler warnings as errors), before any of the tests can even compile.) |
The initial version of react (see #126) let the reactor store all cells in a linked list, then blindly updated all cells whenever any cell was set. This was correct but wasteful. This commit rearchitects the example solution so that: * reactors only keep track of the input cells * all other cells keep track of their dependent compute cells * only dependents get updated when a cell is set Space cost of: * one extra `struct child` allocated per compute2 cell created. * one extra integer field per cell.
Careful - I wouldn't want the maintainers of this track to get in a back-and-forth on #126 (comment) and this, because it's not my place to mediate such a one. But I'm here to let you know that whichever you choose (NULL checks or no NULL checks), it can happen. There is now a commit dedicated to adding the NULL checks, and if y'all decide not to have NULL checks that commit can simply be reverted.
I admit that when I was working on this to submit to the site, I removed |
|
Hi! As I explained, it's not necessary to merge this--the only reason to do this is if it's really important that the example solution not do unnecessary work. Given this fact, and the fact that I need to keep my open PRs I've created clean, I need to close this for the sole purpose of getting it out of my queue. It doesn't mean I've given up on the idea! Y'all are free to reopen it for the purpose of merging it of course. |
|
Hey @petertseng I'm sorry this fell off my radar. I think we'd like this, thanks for your help! |
The initial version of react (see #126) let the reactor store all cells
in a linked list, then blindly updated all cells whenever any cell was
set. This was correct but wasteful.
This commit rearchitects the example solution so that:
Space cost of:
struct childallocated per compute2 cell created.