Skip to content

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Aug 19, 2019

Previously, a variable that was unreachable but which referenced exactly one reachable variable would be computed. This was not the intent! We set variable._indegree to -1 to prevent the variable from being added to the compute queue here:

function postqueue(variable) {
  if (--variable._indegree === 0) {
    queue.push(variable);
  }
}

However, if the unreachable variable was an output of exactly one other reachable variable, its indegree would be erroneously incremented back up to 0 here:

// Compute the indegree of updating variables.
variables.forEach(function(variable) {
  variable._outputs.forEach(variable_increment);
});

By setting variable._indegree to NaN instead of -1, we ensure that this incrementing and decrementing has no effect, and thus the unreachable variable is never computed.

Not computing unreachable variables is important when these variables have side-effects.

@mbostock mbostock requested a review from jashkenas August 19, 2019 20:53
Copy link
Contributor

@jashkenas jashkenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good fix ... but just to fully strap on our belt and suspenders, should we also change the definition of postqueue to be:

function postqueue(variable) {
  if (--variable._indegree === 0 && variable._reachable) {
    queue.push(variable);
  }
}

@mbostock
Copy link
Member Author

That shouldn’t be necessary if the code is correct; just slower. I’ve added a test to confirm the correctness of the fix, and realized it takes exactly two reachable references, not one. (We already had a test for one.)

@mbostock
Copy link
Member Author

Merged in b86e7e9.

@mbostock mbostock closed this Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants