Use Passive flag to schedule onPostCommit

[acdlite]

Instead of calling onPostCommit in a separate phase, we can fire them during the same traversal as the rest of the passive effects.

This works because effects are executed depth-first. So by the time we reach a Profiler node, we'll have already executed all the effects in its subtree.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ff96d8a:

Sandbox	Source
React	Configuration

[sizebot]

Details of bundled changes.

Comparing: 50d9451...ff96d8a

react-dom

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
react-dom.development.js	0.0%	-0.0%	872.83 KB	872.83 KB	199.9 KB	199.9 KB	NODE_DEV
ReactDOMForked-prod.js	0.0%	0.0%	387.1 KB	387.12 KB	71.44 KB	71.45 KB	FB_WWW_PROD
react-dom-server.node.development.js	0.0%	-0.0%	137.06 KB	137.06 KB	36.37 KB	36.37 KB	NODE_DEV
react-dom.production.min.js	0.0%	0.0%	117.72 KB	117.72 KB	38.02 KB	38.03 KB	NODE_PROD
ReactDOMForked-profiling.js	-0.1%	-0.1%	400.91 KB	400.64 KB	73.87 KB	73.83 KB	FB_WWW_PROFILING
react-dom-server.browser.development.js	0.0%	-0.0%	143.14 KB	143.14 KB	36.58 KB	36.57 KB	UMD_DEV
react-dom-server.node.production.min.js	0.0%	-0.0%	20.2 KB	20.2 KB	7.58 KB	7.58 KB	NODE_PROD
ReactDOMTesting-prod.js	0.0%	-0.0%	385.05 KB	385.05 KB	72.38 KB	72.38 KB	FB_WWW_PROD
react-dom-test-utils.production.min.js	0.0%	-0.0%	13.58 KB	13.58 KB	5.22 KB	5.21 KB	NODE_PROD
react-dom.development.js	0.0%	-0.0%	917.24 KB	917.24 KB	202.44 KB	202.44 KB	UMD_DEV
react-dom.production.min.js	0.0%	-0.0%	117.62 KB	117.62 KB	38.71 KB	38.71 KB	UMD_PROD
react-dom.profiling.min.js	0.0%	-0.0%	121.51 KB	121.51 KB	39.92 KB	39.92 KB	UMD_PROFILING
ReactDOMForked-dev.js	-0.1%	-0.0%	989.07 KB	988.34 KB	219.06 KB	218.96 KB	FB_WWW_DEV
react-dom.profiling.min.js	0.0%	-0.0%	121.79 KB	121.79 KB	39.22 KB	39.22 KB	NODE_PROFILING
react-dom-server.browser.production.min.js	0.0%	-0.0%	19.88 KB	19.88 KB	7.46 KB	7.46 KB	UMD_PROD
ReactDOM-dev.js	0.0%	-0.0%	986.84 KB	986.84 KB	219.32 KB	219.32 KB	FB_WWW_DEV
ReactDOM-prod.js	0.0%	-0.0%	385.83 KB	385.83 KB	71.1 KB	71.1 KB	FB_WWW_PROD
react-dom-server.browser.development.js	0.0%	-0.0%	135.79 KB	135.79 KB	36.13 KB	36.12 KB	NODE_DEV
ReactDOM-profiling.js	0.0%	-0.0%	399.04 KB	399.04 KB	73.53 KB	73.53 KB	FB_WWW_PROFILING
ReactDOMServer-prod.js	0.0%	-0.0%	47.3 KB	47.3 KB	11.04 KB	11.04 KB	FB_WWW_PROD
react-dom-test-utils.development.js	0.0%	-0.0%	70.21 KB	70.21 KB	19.11 KB	19.11 KB	UMD_DEV

Size changes (stable)

Generated by 🚫 dangerJS against ff96d8a

[sizebot]

	Warnings
⚠️	Failed to fetch build artifacts for base commit: 50d9451

Size changes (experimental)

Generated by 🚫 dangerJS against ff96d8a

[bvaughn]

Cool 👍

I think we could address the TODO by moving the Passive flag update into the complete phase, where we could conditionally set it based on subtreeFlags & Passive. The one case that wouldn't account for would be deletions, but I think we could account for that by also checking if flags & Deletion.

I think some Profiler tests would fail b'c they were written to expect the (broken) behavior of onPostCommit getting called even when there were no passive effects...

Maybe I'll tackle this as a follow up PR. It's possible I'm over simplifying something. :)

[bvaughn]

What does this comment mean?

[acdlite]

Roughly this (typed it out fast, might be a mistake)

let nearestProfiler = null;

function commitWork(finishedWork) {
  switch (finishedWork.tag) {
    case Profiler: {
      // Can push/pop values onto stack before entering/leaving a subtree, like
      // how we do in the render phase, except since this is a recursive
      // function we can use the regular JS stack instead of our fake
      // fiber stack.
      //
      // This is the code for Profiler. We'd do something similar for Offscreen.
      if (finishedWork.subtreeFlags & FlagForThisPhase) {
        const prevProfilerOnStack = nearestProfiler;
        nearestProfiler = finishedWork;

        try {
          let child = finishedWork.child;
          while (child !== null) {
            commitWork(child);
            child = child.sibling;
          }
        } finally {
          nearestProfiler = prevProfilerOnStack;
        }
      }

      commitProfilerEffect(finishedWork);
      break;
    }
    default: {
      // Normal path, no pushing or popping
      if (finishedWork.subtreeFlags & FlagForThisPhase) {
        let child = finishedWork.child;
        while (child !== null) {
          commitWork(child);
          child = child.sibling;
        }
      }

      switch (finishedWork.tag) {
        case FunctionComponent: {
          // Can reference `nearestProfiler` here
          commitFunctionComponent(finishedWork, nearestProfiler);
          break;
        }
        // ... and so on
      }
      break;
    }
  }
}

[bvaughn]

Happy to pick this TODO up if you haven't already started on it. Let me know.

[bvaughn]

Circling back on this.

It's possible that I'm misunderstanding what you're suggesting, but I think the pseudo-code solution is missing a few things.

There are two places we walk the return path to find the nearest Profiler ancestor:

• commitLifeCycles to bubble up layout effect durations from one Profiler to the next.
• commitProfilerPassiveEffect to bubble up passive effect durations from one Profiler to the next.

Neither of these functions are recursive. The work loop functions that call them are (e.g. commitLayoutEffects and flushPassiveMountEffects) so maybe that's what you meant to show instead with the above pseudo-code example?

There's another snag though. These durations bubble up one Profiler at a time. The "TODO" code in question is just bubbling the durations from the current Profiler to the next nearest (ancestor) Profiler. If we used a stack for this, the top of the stack would always just point to the current Profiler (which we already have, since its the current Fiber we're working on). So we'd need to maintain pointers to the top two Profilers in the stack.

This could work, but it's not as clean an implementation as you were perhaps thinking? I'll post a PR.

[bvaughn]

#19895

[acdlite]

The work loop functions that call them are (e.g. commitLayoutEffects and flushPassiveMountEffects) so maybe that's what you meant to show instead with the above pseudo-code example?

Yeah commitWork in my example is meant to correspond to commitLayoutEffects et al

[bvaughn]

Gotcha.

The naming of commitWork confused the heck out of me then, given where the TODO comments were previously (in ReactFiberCommitWork - which also has a function called commitWork)

[acdlite]

Maybe I'll tackle this as a follow up PR. It's possible I'm over simplifying something. :)

I'm about to open this PR :D Will send in a bit

[bvaughn]

Ahh no I've already started it ;)

But that's fine if you have too. The slowest part is just spot checking the few Profiler tests that fail (as they should) to make sure none are legit failures.

[acdlite]

The slowest part is just spot checking the few Profiler tests that fail (as they should) to make sure none are legit failures.

Yeah that's the part I hadn't finished yet. How about I'll push what I've got and let you update the tests to be sure nothing is wrong?

[acdlite]

@bvaughn Here's the follow-up PR #19863

[bvaughn]

😆 I had already done the other part too. Updating the tests is the boring part.

It's fine. We can sort it out on Monday!