Import React Concurrent Mode Profiler

[taneliang]

Summary

This PR imports the concurrent mode profiler project into packages/react-devtools-scheduling-profiler, with history 🎉

Broadly follows the approach of #16381.

Resolves MLH-Fellowship/scheduling-profiler-prototype#134.

cc @bvaughn

Import Details

Notes

• Our new build and start scripts were failing with this error: No "exports" main resolved in @babel/helper-compilation-targets/package.json babel/babel#11216 (comment). I've followed the maintainer's recommendation and updated all @babel/* packages across the repository, excluding fixtures. We can easily revert this (i.e. babe03c) if there's a better solution.
• Webpack config is based on react-devtools-shell's.
• Dependencies used have been upgraded to the latest versions. css-loader config differences can be backported into the other devtools packages later.
• Unlike the other devtools packages, we generate our index.html file into the dist folder using HtmlWebpackPlugin. This means that the dist folder can be served directly, and can likely be deployed to Vercel without a need for a now.json file.

Major differences with the version in the original repo (full diff):

• Parcel has been replaced with Webpack for better integration with the rest of the React repo.
• The sample profile auto-loaded in DEV for convenience is now dynamically imported. It's now trivial to add a demo profile feature for end users (Craft a custom performance profile to "demo" MLH-Fellowship/scheduling-profiler-prototype#82).
• index.html is now automatically generated by html-webpack-plugin instead of being based on a template file.
• Page title has been renamed to "React Concurrent Mode Profiler".

Process

Similar to #16381, the goal of this import is to preserve history from https://github.com/MLH-Fellowship/scheduling-profiler-prototype. Verified that git log --follow worked as expected on imported files.

Prepare fork for merge

mkdir profiler-merge
cd profiler-merge

git init .
git commit -m "Initializing empty merge repo" --allow-empty  

git remote add -f profiler-merge [email protected]:MLH-Fellowship/scheduling-profiler-prototype.git
git merge profiler-merge/master --allow-unrelated-histories

mkdir -p packages/react-scheduling-profiler
mv * packages/react-devtools-scheduling-profiler
mv .* packages/react-devtools-scheduling-profiler
mv packages/react-devtools-scheduling-profiler/.git .
# Commit

Import into React

cd ../react

git remote add profiler-importable ../profiler-merge
git fetch --all

git checkout -b scheduling-profiler-import
git merge --allow-unrelated-histories profiler-importable/importable
git remote rm profiler-importable

Checklist

• Remove extraneous project-specific files (.github, .vscode, etc)
• Integrate package.json with monorepo and remove yarn.lock
• Fix build/start
• Integrate with linter
• Fix lint
• Integrate with Flow
• Fix Flow types, if required
• Integrate with tests
• Fix tests
• Add FB copyright header
• Fix CI

Test Plan

• CI
• In react-scheduling-profiler package:
  • Test prod build
    1. yarn build
       
    2. cd dist
    3. serve (using https://www.npmjs.com/package/serve)
       
  • Test development:
    1. yarn start works
       
    2. Test hot reloading works

[taneliang]

let's move the deps upgrade to its own PR? Sorry to ask you to do this.

No worries! I had planned for that so the Babel upgrade commit could be cherry picked and reverted easily 😆 The PR is opened at #19647. I'll revert commit a633ac3 in this PR once it's merged.

[bvaughn]

I think I've reviewed everything but the View classes.

Trying to find time to do those too but have had a ton of meetings today.

[bvaughn]

I know I wrote this originally, but as I look at it again– I realize the "show" function is a little brittle. If a menu is already being shown, calling show again will leave that menu hanging.

Seems like we should check if currentHideFn !=== null and if so– call hideMenu before continuing to show the new one. Thoughts?

[taneliang]

Ah, nice catch! I'll do that.

Should I also make this change in react-devtools-shared's implementation too? Also, would it be a good idea to merge the 2 context menu implementations after this PR? I think the only differences are that the scheduling profiler's implementation has an extra onChange callback (from your prototype) and slightly more comprehensive Flow types (which I added), as well as some CSS changes.

[bvaughn]

Yeah if the same bug exists in react-devtools-shared' it would be nice to do a fix for that as well (maybe as a separate PR tho 😁) Flow typing improvements also welcome!

Let's not do any code-sharing yet.

[bvaughn]

nit: Can we name this something more semantically meaningful? The current name is too similar to rectIntersectsRect.

Maybe something like intersectionOfRects (to match the naming of unionOfRects below)

[taneliang]

Makes sense! When I wrote this I didn't want to call it intersectionOfRects because I thought it'd be more confusing that unionOfRects accepts any number of rects whereas this only accepts 2. But I agree that intersectionOfRects will be easier to read, especially since it's often called close to rectIntersectsRect. I'll rename it

[bvaughn]

This conversion seems not very useful. Can we get rid of it?

Seems like passing the view around would be the same in terms of property access for the frame bit, and would remove the unnecessary object wrapper and view access for other properties.

[taneliang]

This conversion seems not very useful. Can we get rid of it?

I don't think we can 😆

Because the layouters are composable (via createComposedLayout), they need some way to pass the intermediate frames to each other. If we get rid of this LayoutInfo wrapper and store the intermediate frames in the views, we have some problems because:

1. Invalidated views are redrawn the next time CanvasPage's layout effects are flushed. Because the View class's setFrame method invalidates the view's contents (by calling setsNeedsDisplay) if the frame is changed, we only want to call setFrame with the final frame, so that the view will only be redrawn if the frame actually changed.

   Without the LayoutInfo wrapper, the intermediate frames will trigger a lot of unnecessary invalidation and thus many views will be unnecessarily redrawn.

2. View subclasses can override the setFrame method and do arbitrary things, including updating its state. For example, HorizontalPanAndZoomView and VerticalScrollView re-clamp their offsets and zoom levels when the frame changes, so that e.g. resizing the browser window don't cause black areas to appear. If this is done with intermediate frames, this clamping can cause bugs, e.g. if an intermediate frame had a much wider width than the final one, resizing the browser window may cause you to suddenly scroll left as the offset got clamped with that intermediate width.

This LayoutInfo wrapper was the best way I could think of to solve this. Is there a better way to solve this?

[bvaughn]

Ditto for this mapping.

[bvaughn]

We should still pass a deps array to useCallback. That array should just be empty.

Without the deps array, useCallback will just always pass the new function every time it's called.

[bvaughn]

This is coming along nicely.

This morning I looked through the View classes. I have some initial high-level thoughts which I'll leave as comments.

[bvaughn]

Did we intentionally remove the loading spinner state? Looks like the browser is back to looking frozen when importing large profiles again.

[kartikcho]

@bvaughn the new import page that you saw in the meeting was a WIP PR that is on hold until this is merged. I'll be making a PR here soon after this is merged to get that in!

[bvaughn]

Ah! I mixed PRs up then.

Sounds good. Thanks @kartikcho

[bvaughn]

This approach makes me a little uneasy, because subviews is managed by the superclass– but the subclass has hard-coded assumptions about the size and ordering.

I wonder if you considered making subviews a getter method that the subclasses had to implement rather than calling addSubview to register? Then this casting could be unnecessary, e.g.

export class ResizableSplitView extends View {
  // No casting or Array access necessary:
  _bottomSubview: View;
  _resizeBar: View;
  _topSubview: View;

  // Memoized array:
  _subviews: View[];

  constructor(
    surface: Surface,
    frame: Rect,
    topSubview: View,
    bottomSubview: View
  ) {
    super(surface, frame, noopLayout);

    this._bottomSubview = bottomSubview;
    this._resizeBar = new ResizeBar(surface, frame);
    this._topSubview = topSubview;

    this._subviews = [_topSubview, _resizeBar, _bottomSubview];

    // ...
  }

  getSubviews(): View[] {
    this._subviews;
  }
}

(Same general idea goes for the other views too, but I won't repeat the comment.)

[taneliang]

Yep, I actually implemented something similar initially. Unfortunately, I didn't think it was very clean as it forced all view subclasses to correctly handle and maintain subview/superview relationships. I figured it'll be better to centralize all of that in the base View class. What do you think?

Regarding this particular issue, maybe it'll be better if we stored a reference to the views in the subclass? It's definitely still not great though. Seems like there are some deep seated issues in this architecture 😞

export class ResizableSplitView extends View {
  // No casting or Array access necessary:
  _bottomSubview: View;
  _resizeBar: View;
  _topSubview: View;

  constructor(
    surface: Surface,
    frame: Rect,
    topSubview: View,
    bottomSubview: View
  ) {
    super(surface, frame, noopLayout);

    this._bottomSubview = bottomSubview;
    this._resizeBar = new ResizeBar(surface, frame);
    this._topSubview = topSubview;

    this.addSubview(this._topSubview);
    this.addSubview(this._resizeBar);
    this.addSubview(this._bottomSubview);

    // ...
  }
  // ...
}

[bvaughn]

The way handleInteraction and handleInteractionAndPropagateToSubviews work together seems like an unfortunate constraint that results from using inheritance.

I guess we could enforce that handleInteraction was never called directly, e.g.

class Foo {
  _handleInteractionLock: boolean = true;

  handleInteraction(interaction: Interaction) {
    if (this._handleInteractionLock) {
      throw Error("Do not call handleInteraction() directly");
    }

    // ...
  }

  handleInteractionAndPropagateToSubviews(interaction: Interaction) {
    this._handleInteractionLock = false;
    try {
      this.handleInteraction(interaction);
    } finally {
      this._handleInteractionLock = true;
    }
  }
}

But this feels like overkill.

I wonder if you considered an alternate design here that used composition instead, to better enforce the API between both sides? Then the separation might be a little clearer.

I haven't thought about this deeply, but maybe something like

class ViewManager {
  _view: View;

  constructor(view: View) {
    this._view = view;
  }

  handleInteraction(interaction: Interaction) {
    const view = this._view;
    view.handleInteraction(interaction);
    view
      .getSubviews()
      .forEach((subview) =>
        subview.handleInteractionAndPropagateToSubviews(interaction)
      );
  }

  // ...
}

class ExampleView implements View {
  constructor() {
    this._viewManager = new ViewManager(this);
  }

  getSubviews(): View[] {
    // ...
  }

  handleInteraction(interaction: Interaction) {
    // ...
  }

  // ...
}

[taneliang]

Hmm, maybe I'm not understanding your ViewManager design correctly. If the ViewManager class is an implementation detail of the View class, won't View still need to implement all its existing methods, including handleInteractionAndPropagateToSubviews?

What about this alternative design:

1. Turn View into an interface that only implements e.g. draw, handleInteraction, etc.
2. Define a new public ViewManager class that handles the invalidation and interaction propagation logic.

CanvasPage will then instantiate ViewManagers and inject concrete View implementations into them.

This will mean that the manager will handle subViewManager/superViewManager relationships. Also, View implementations that instantiate their own subviews (e.g. ResizableSplitView, FlamechartView) will need to know how to instantiate ViewManagers, which doesn't feel right.

[bvaughn]

Can we give these marks a different visual style, to set them apart from the e.g. state updates and suspends? Maybe something like:

Suggested change

	_drawSingleMark(
	context: CanvasRenderingContext2D,
	rect: Rect,
	mark: UserTimingMark,
	baseY: number,
	scaleFactor: number,
	showHoverHighlight: boolean,
	) {
	const {frame} = this;
	const {timestamp} = mark;
	const x = timestampToPosition(timestamp, scaleFactor, frame);
	const radius = EVENT_DIAMETER / 2;
	const markRect: Rect = {
	origin: {
	x: x - radius,
	y: baseY,
	},
	size: {width: EVENT_DIAMETER, height: EVENT_DIAMETER},
	};
	if (!rectIntersectsRect(markRect, rect)) {
	return; // Not in view
	}
	const fillStyle = showHoverHighlight
	? COLORS.USER_TIMING_HOVER
	: COLORS.USER_TIMING;
	if (fillStyle !== null) {
	const y = markRect.origin.y + radius;
	context.beginPath();
	context.fillStyle = fillStyle;
	context.arc(x, y, radius, 0, 2 * Math.PI);
	context.fill();
	}
	}
	_drawSingleMark(
	context: CanvasRenderingContext2D,
	rect: Rect,
	mark: UserTimingMark,
	baseY: number,
	scaleFactor: number,
	showHoverHighlight: boolean,
	) {
	const {frame} = this;
	const {timestamp} = mark;
	const x = timestampToPosition(timestamp, scaleFactor, frame);
	const size = USER_TIMING_MARK_SIZE;
	const halfSize = size / 2;
	const markRect: Rect = {
	origin: {
	x: x - halfSize,
	y: baseY,
	},
	size: {width: size, height: size},
	};
	if (!rectIntersectsRect(markRect, rect)) {
	return; // Not in view
	}
	const fillStyle = showHoverHighlight
	? COLORS.USER_TIMING_HOVER
	: COLORS.USER_TIMING;
	if (fillStyle !== null) {
	const y = baseY + halfSize;
	context.beginPath();
	context.fillStyle = fillStyle;
	context.moveTo(x, y - halfSize);
	context.lineTo(x + halfSize, y);
	context.lineTo(x, y + halfSize);
	context.lineTo(x - halfSize, y);
	context.fill();
	}
	}

The above proposal also involves adding a new constant to content-views/constants.js:

export const USER_TIMING_MARK_SIZE = 8;

[bvaughn]

I think this is okay to merge as-is. I have some small suggestions (mentioned above) that could be made in a follow up PR, either by myself or you folks.

I want to land #19647 first though, on its own. Then we can land this one.

[bvaughn]

Looks like the yarn.lock file now conflicts.

[bvaughn]

I don't have permissions to push to this repo, so I can't fix it. @kartikcho can you merge master and resolve the conflict?

[taneliang]

I think this is okay to merge as-is.

Exciting! 🥳

I have some small suggestions (mentioned above) that could be made in a follow up PR, either by myself or you folks.

Thanks for the suggestions! I'll look at them in greater detail tomorrow as it's late here, but I really like the idea of having a ViewManager 😄

[bvaughn]

Excellent work @taneliang and @kartikcho!

I'm going to merge this now and we/I can follow up on some of the remaining items afterward.