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

Refactored backend to remove recursion #385

Closed
wants to merge 1 commit into from
Closed

Conversation

bvaughn
Copy link
Owner

@bvaughn bvaughn commented Aug 24, 2019

DevTools doesn't currently handle extremely deep trees (facebook/react#16491) or extremely wide trees (facebook/react#16501) very well, due to recursion in the backend interface. This PR removes most of that.

As a sanity test, I confirmed that after this refactor, DevTools was able to handle the following:

const Child = ({ children = null }) => children;

const Deep = () => {
  let children = null;
  for (let i = 0; i < 15000; i++) {
    children = <Child>{children}</Child>;
  }
  return children;
};

const Wide = () => {
  let children = [];
  for (let i = 0; i < 15000; i++) {
    children.push(<Child key={i} />);
  }
  return children;
};

@bvaughn bvaughn changed the title Refactored backend renderer to remove most of the recursion Refactored backend to remove recursion Aug 24, 2019
//
// TRICKY
// Although this method is recursive, refactoring it to be iterative would add a lot of complexity.
// Since it is not expected to operate on huge parts of the tree at once, it's probably okay?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it not expected?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe I misread the function? 🙂 I'm not as familiar with the suspense logic since I didn't write it. I just skimmed it and read the header comment:

// Returns whether closest unfiltered fiber parent needs to reset its child list.

It seemed like it didn't recurse as greedily as the mount/unmount functions. Looking at our tests it also seemed like the function didn't recurse as deeply as the others (primarily mount) but that could just be due to it being a small sample size.

Think we should rewrite it also? I would have just done it initially, except that it seemed trickier than the other two since it also depends on its own return value.

Choose a reason for hiding this comment

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

It seems that traversing deep trees causes this function to stack overflow. See this example

const Child = ({ children = null }) => children;

const Deep = () => {
  const [condition, setCondition] = useState(false);

  let children = null;
  for (let i = 0; i < 15000; i++) {
    children = <Child>{children}</Child>;
  }
  return (
    <div>
      <button onClick={() => setCondition(c => !c)}>Rerender</button>
      {children}
    </div>
  );
};

The initial render works, but rerendering the component recurses too deeply and overflows. Adding the same rerender button on the Wide component works fine though.

@bvaughn
Copy link
Owner Author

bvaughn commented Aug 30, 2019

Going to migrate this PR to the React repo -> facebook/react#16627

@bvaughn bvaughn closed this Aug 30, 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.

3 participants