feat: detect unnecessary render and warn user #468

guvenkaranfil · 2024-03-14T01:38:09Z

Summary

This draft PR is intended to start providing a way to detect unnecessary renders as described here

Test plan

I have added a test file called UnneccassaryRender.perf-test which is intended to unnecessarily render on the component and see the warning

changeset-bot · 2024-03-14T01:38:13Z

⚠️ No Changeset found

Latest commit: 59e0672

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

guvenkaranfil · 2024-03-14T02:30:49Z

I found something very interesting while testing the implementation @mdjastrzebski

For Unncessary Component test, wrapping state updating inside a timeout cause not finding the unnecessary renders instead of updating directly.

const handlePress = () => { setCount((c) => c + 1); };

with setTimeOut

const handlePress = () => { setTimeout(() => setCount((c) => c + 1), 10); };

guvenkaranfil · 2024-03-14T02:37:04Z

Another thing I came up with while testing is currently screen variable has been set in the outer scope.

On the first render screen variable is not set yet. On the second onRender callback screen variable is being set. That means first two onRender callbacks we can not do any comparison. So on the third callback can be comparasion because we have screen object and previously saved component representation

I am not sure I am missing any point you have described on the discussion issue @mdjastrzebski

mdjastrzebski · 2024-03-14T11:19:16Z

@guvenkaranfil good first step. As a next step you should probably modify measurePerformance function to capture the screen.toJSON() trees (only for RN!) to some array during the first run. Then after the 1st run compare the consecutive JSON trees to find out if they are the same.

guvenkaranfil · 2024-03-15T23:05:13Z

@mdjastrzebski I've just updated the implementation based on capturing component representations into an array and compare after.

Then after the 1st run compare the consecutive JSON trees to find out if they are the same.

Why not we would like to do comparison after 1st run instead of like end of the test like hasTooLateRender

mdjastrzebski · 2024-03-18T20:11:11Z

@guvenkaranfil The comparison can be made after all runs, that is not that important.

The important factors are:

Gather the toJSON tree during only the first count, and avoid capturing then during any subsequent runs in order not to affect the render times.
The first run is ideal here, as it will usually be discarded (warmupRuns: 1 is the default setup).

mdjastrzebski · 2024-03-18T20:15:25Z

packages/measure/src/testing-library.ts

+export function isRNTLRunning(): boolean {
+  return RNTL != null;
+}


Suggested change

export function isRNTLRunning(): boolean {

return RNTL != null;

}

export function getTestingLibrary(): boolean {

if (typeof config.testingLibrary === 'string') {

config.testingLibrary.

}

if (RNTL != null) {

return "react-native";

}

if (RTL != null) {

return "react";

}

return null;

}

mdjastrzebski · 2024-03-18T20:15:40Z

test-apps/native/src/UnneccassaryRender.perf-test.tsx

+import { fireEvent, screen } from '@testing-library/react-native';
+import { measurePerformance } from 'reassure';
+
+const UnnecessaryComponent = () => {


[nit]

Suggested change

const UnnecessaryComponent = () => {

const UnnecessaryRendersComponent = () => {

mdjastrzebski · 2024-03-18T20:16:36Z

packages/measure/src/measure-renders.tsx

 export async function measureRender(ui: React.ReactElement, options?: MeasureOptions): Promise<MeasureResults> {
  const runs = options?.runs ?? config.runs;
  const scenario = options?.scenario;
  const warmupRuns = options?.warmupRuns ?? config.warmupRuns;

  const { render, cleanup } = resolveTestingLibrary();
+  const isCurrentlyRNTL = isRNTLRunning();


const testingLibrary = getTestingLibrary();

mdjastrzebski · 2024-03-18T20:19:12Z

packages/measure/src/measure-renders.tsx

+      if (isCurrentlyRNTL && screen !== undefined && i === isFirstRun && screen.toJSON()) {
+        jsonRepresentations.push(JSON.stringify(screen.toJSON()));
+      }


Suggested change

if (isCurrentlyRNTL && screen !== undefined && i === isFirstRun && screen.toJSON()) {

jsonRepresentations.push(JSON.stringify(screen.toJSON()));

}

if (i === 0 && testingLibrary === 'react-native' && screen != null) {

const json = screen.toJSON();

jsonRepresentations.push(JSON.stringify(json));

}

For work in progress let's keep JSON.stringify. For actual merge perhaps we will add a simple function to compare the trees using DFS (depth-first search) algo.

mdjastrzebski · 2024-03-18T20:23:14Z

packages/measure/src/measure-renders.tsx

@@ -55,8 +65,7 @@ export async function measureRender(ui: React.ReactElement, options?: MeasureOpt
    };

    const uiToRender = buildUiToRender(ui, handleRender, options?.wrapper);
-    const screen = render(uiToRender);
-
+    screen = render(uiToRender);


I wonder: doesn't this generate a race condition in which handleRender is called earlier than the actual render returns?

Pls add logging and check this. Perhaps, for the first render we might need to manually capture screen.toJSON() just after calling render.

Actually this is the part I am so confused. Since handleRender defined and called before screen how we can capture the json 🤔

In addition, Imagine we have a component that updating state on useEffect.

handleRender will be called but screen will be not initialized yet. So we can not capture the state update even the state update might not effect the UI. For code demonstration;

`const UnnecessaryRendersComponent = () => {
const [name, setname] = React.useState('');

React.useEffect(() => {
setname('Hello');
}, []);

return ;
};`

const screen = render(...); if (i === 0 && testingLibrary === 'react-native') { const firstJson = screen.toJSON(); }

That should work

regarding useEffects with no async delay. We would have to test if they happen before initial render returns. It might be so, but that would also by a symptom of a problem, that when component has all the data at hand (i.e. no async operations happen) it is triggering re-render using useEffect instead of rendering it once correctly.

Next we will do the tree comparison between the final initial render and subsequent deplayed (async) re-renders. These we can simpy call "unnecessary renders" (perhaps you have some better name) and are the original scope of this PR.

So actually if useEffect contains some sync operations React immediately schedule for re-render operation but async operation React does not schedule a re-render operation it waits for async operation for initially rendering the component. Did I get it correctly 🤔

Yes, async operation will probably end after initial render(s).

To sum up we have to cases when we want to warn users:

More than 1 initial render: this means that they put some useEffect which triggers immediate re-render. From performance perspective it should be transformed into a single render, as it's is not waiting for addtional data (as it is sync).

Two subsequent renders giving the same JSON output. In such case the render did emit the same output and hence could be optimized by improving code, probably be adding memoization, improving hooks subscriptions, etc.

I see, I updated the implementation based on that. As a result we have an object holds both situations.

redundantRender: { initial: number; update: number; };

initial is used for first and update is for second one. We know also put the information to output.json as well. And as comparison we do in Count changes its apply for Redundant Render

But I do have a question for comparing subsequentially I've used pretty-format package. Do you think it is okay or we should write a function as you said earlier which uses dfs to go through and look for match @mdjastrzebski

mdjastrzebski · 2024-03-18T20:23:57Z

packages/measure/src/measure-renders.tsx

+    jsonRepresentations.every((json) => json === jsonRepresentations.at(0))
+  ) {
+    const testName = expect.getState().currentTestName;
+    logger.warn(`test "${testName}" has unnecessary renders. Please update your code to avoid unnecessary renders.`);


Optimally we would like to surface this into output JSON file, and subsequently generated MD and screen reports.

Sounds good! Basically we would like to have reporting like Count changes etc. right 🤔

I do save the result into output.json now and show the comparison like Count Changes shows

mdjastrzebski · 2024-03-18T20:24:52Z

packages/measure/src/measure-renders.tsx

@@ -69,6 +78,19 @@ export async function measureRender(ui: React.ReactElement, options?: MeasureOpt
    runResults.push({ duration, count });
  }

+  if (
+    jsonRepresentations.length >= minRecordToCompare &&
+    jsonRepresentations.every((json) => json === jsonRepresentations.at(0))


The check should compare each pair of subsequent renders results, not each vs the first one

guvenkaranfil · 2024-03-26T02:06:48Z

@mdjastrzebski Have you had a chance to look updated parts 🤔

mdjastrzebski · 2024-03-26T13:59:56Z

@guvenkaranfil Thanks for your patience. I'll try to check it soon.

mdjastrzebski · 2024-03-26T14:08:46Z

test-apps/native/src/UnneccassaryRender.perf-test.tsx

+test('Unncessary Renders Component', async () => {
+  const scenario = async () => {
+    const button = screen.getByText('Action');
+    await screen.findByText('Count: 0');


THe findBy seems unnecessary to the test.

mdjastrzebski · 2024-03-26T14:09:16Z

packages/measure/src/types.ts

@@ -25,4 +25,9 @@ export interface MeasureResults {

  /** Array of measured render/execution count for each run */
  counts: number[];
+
+  redundantRender: {


Suggested change

redundantRender: {

redundantRenders: {

It would make sense to only add this for measurePerformance functio nand not for measureFunction. Therefore let's have 👍🏻

interface MeasurePerfromanceResults extends MeasureResult { redundantRenders: RendundantRendersResults; } interface RendundantRendersResults { initialRenders: number; updates: number; }

mdjastrzebski · 2024-03-26T14:14:00Z

packages/measure/src/measure-renders.tsx

  if (hasTooLateRender) {
    const testName = expect.getState().currentTestName;
    logger.warn(
      `test "${testName}" still re-renders after test scenario finished.\n\nPlease update your code to wait for all renders to finish.`
    );
  }

-  return processRunResults(runResults, warmupRuns);
+  return processRunResults(runResults, warmupRuns, components);


instead of passing compoennt for analysis to processRunResults which is also used by measureFunction lets call function to detect redundant renders before calling it.

mdjastrzebski · 2024-03-26T14:15:18Z

packages/measure/src/measure-renders.tsx


  showFlagsOutputIfNeeded();

  const runResults: RunResult[] = [];
  let hasTooLateRender = false;
+  let hasUnnecessaryRender = false;


There seems to be no code chaning this value.

I would suggest renaming this to (+ change type to number)

let intialRendersCount = 0'

mdjastrzebski · 2024-03-26T14:19:19Z

packages/measure/src/measure-renders.tsx


  showFlagsOutputIfNeeded();

  const runResults: RunResult[] = [];
  let hasTooLateRender = false;
+  let hasUnnecessaryRender = false;
+  let components: RenderMeasureResult = [];


The types does not seem correct.

Suggested change

let components: RenderMeasureResult = [];

type ToJsonTree = ReactTestRendererJSON | ReactTestRendererJSON[] | null;

let renderJsonTrees: ToJsonTree = [];

mdjastrzebski · 2024-03-26T14:21:19Z

packages/measure/src/measure-renders.tsx


  showFlagsOutputIfNeeded();

  const runResults: RunResult[] = [];
  let hasTooLateRender = false;
+  let hasUnnecessaryRender = false;
+  let components: RenderMeasureResult = [];
  for (let i = 0; i < runs + warmupRuns; i += 1) {
    let duration = 0;
    let count = 0;
    let isFinished = false;



detect initial rendundant rerenders:

// Count renders happening before `render` returns. More than 1 render means there are redundant renders for things like `useEffect()`. if (screen == null) { intialRendersCount += 1; }

Actually I believe we do not need to hold intialRendersCount because in the end I calculate the initial redundant renders by counting undefined values on the json array

mdjastrzebski · 2024-03-26T14:21:51Z

packages/measure/src/measure-renders.tsx

@@ -69,14 +81,19 @@ export async function measureRender(ui: React.ReactElement, options?: MeasureOpt
    runResults.push({ duration, count });
  }

+  if (hasUnnecessaryRender) {


Suggested change

if (hasUnnecessaryRender) {

if (initialRenderCount > 1) {

mdjastrzebski · 2024-03-26T14:22:16Z

packages/measure/src/measure-renders.tsx

@@ -69,14 +81,19 @@ export async function measureRender(ui: React.ReactElement, options?: MeasureOpt
    runResults.push({ duration, count });
  }

+  if (hasUnnecessaryRender) {
+    const testName = expect.getState().currentTestName;
+    logger.warn(`test "${testName}" has unnecessary renders. Please update your code to avoid unnecessary renders.`);


Suggested change

logger.warn(`test "${testName}" has unnecessary renders. Please update your code to avoid unnecessary renders.`);

logger.warn(`test "${testName}" has ${initialRendersCount - 1} unnecessary initial render(s). Please update your code to avoid unnecessary renders.`);

mdjastrzebski · 2024-03-26T14:22:57Z

packages/measure/src/measure-helpers.tsx

+  const formatOptionsZeroIndent = { ...FORMAT_OPTIONS, indent: 0 };
+  let count = 0;
+
+  for (let i = 0; i < components.length - 1; i++) {


mdjastrzebski

Looks like you are going in the good direction. I've suggested some tweaks to actual format of rendundantRenders object, as well as moving things around a bit.

Next step would be to extend the Markdown and Console reports to include info about rendundant re-renders (both initial and updates).

guvenkaranfil · 2024-03-26T15:22:08Z

Hello @mdjastrzebski, thanks for detailed feedback. I will follow up your comments and update accordingly. I believe with latest addition we show redundant render information both initial and update.

guvenkaranfil · 2024-03-29T03:20:55Z

@mdjastrzebski Should we separate including redundant render information into markdown by types initial and update as we do right now for the console

guvenkaranfil · 2024-04-01T07:14:59Z

Hi @mdjastrzebski, I believe it is better to put init and update. What do you think?

guvenkaranfil · 2024-04-11T17:11:57Z

Hi @mdjastrzebski, Have you chance to look latest changes? I'd love to get update finalize the PR. Could you please tell me the steps we would need to take like adding additional tests to verify changes etc.

mdjastrzebski · 2024-04-14T14:14:38Z

@guvenkaranfil Thank you for your patience with this PR, I didn't have time to work on it yet. From my perspective, I plan to merge it ~soon (before end of April), after doing final review and probably some minor tweaks by hand. You do not need to make any more changes.

…#433)

guvenkaranfil · 2024-05-02T19:45:57Z

@mdjastrzebski Is there anything to help to forward the PR?

mdjastrzebski · 2024-05-02T21:28:56Z

@guvenkaranfil This PR will be merged, I just need to find some spare to review it properly.

guvenkaranfil · 2024-05-02T21:54:41Z

@mdjastrzebski Okay thank you just let me know if you need me to update anything. I cant wait to use this feature to use my own projects

…nnecessary rendering

… now

…t state and compare changes between states using dfs

….json and show comparation results in the end of the test

mdjastrzebski · 2024-05-03T10:14:42Z

Slight progress: rebased to v1 branch, as v1 release is imminent

guvenkaranfil mentioned this pull request Mar 14, 2024

[FEATURE] Detect unnecessary renders #225

Open

mdjastrzebski reviewed Mar 18, 2024

View reviewed changes

mdjastrzebski reviewed Mar 26, 2024

View reviewed changes

mdjastrzebski requested changes Mar 26, 2024

View reviewed changes

guvenkaranfil requested a review from mdjastrzebski April 5, 2024 12:49

mdjastrzebski added 7 commits April 25, 2024 10:14

refactor: rename meassurePerformance to measureRenders (callstack…

52c71c2

…#433)

chore: v1.0.0-beta

1e8b751

docs: update changelogs

f581ea2

docs: migration guide

b57641f

chore: update snapshots

cc95dac

chore: update node ver on GH

84baca4

chore: v1.0.0-rc

f106047

Guven Karanfil and others added 8 commits May 3, 2024 11:58

feat: detect unnecessary render and warn user

b1b39c4

feat: capture json representations in an array to compare after for u…

b461db4

…nnecessary rendering

fix: get current testingLibrary for comparasion only react-native for…

5edbbf2

… now

fix: save json representation instead of string for rendered componen…

2e402ce

…t state and compare changes between states using dfs

fix: test name

a2e54f7

fix: update comparasion function and save compare results into output…

1c942c6

….json and show comparation results in the end of the test

fix: update interface and variable names based on pr recommendatations

ea2eb56

chore: rebase to v1

59e0672

mdjastrzebski force-pushed the feat/detectUnnecessaryRender branch from 0fbea97 to 59e0672 Compare May 3, 2024 10:14

mdjastrzebski changed the base branch from main to v1 May 3, 2024 10:14

mdjastrzebski marked this pull request as ready for review May 3, 2024 10:15

mdjastrzebski force-pushed the v1 branch from 92b6d3e to c00fb32 Compare May 4, 2024 07:31

-export function isRNTLRunning(): boolean {
-  return RNTL != null;
-}
+export function getTestingLibrary(): boolean {
+  if (typeof config.testingLibrary === 'string') {
+    config.testingLibrary.
+  }
+  if (RNTL != null) {
+    return "react-native";
+  }
+  if (RTL != null) {
+    return "react";
+  }
+  return null;
+}

	const UnnecessaryComponent = () => {
	const UnnecessaryRendersComponent = () => {

-      if (isCurrentlyRNTL && screen !== undefined && i === isFirstRun && screen.toJSON()) {
-        jsonRepresentations.push(JSON.stringify(screen.toJSON()));
-      }
+      if (i === 0 && testingLibrary === 'react-native' && screen != null) {
+        const json = screen.toJSON();
+        jsonRepresentations.push(JSON.stringify(json));
+      }

-  let components: RenderMeasureResult = [];
+type ToJsonTree = ReactTestRendererJSON | ReactTestRendererJSON[] | null;
+let renderJsonTrees: ToJsonTree = [];

	logger.warn(`test "${testName}" has unnecessary renders. Please update your code to avoid unnecessary renders.`);
	logger.warn(`test "${testName}" has ${initialRendersCount - 1} unnecessary initial render(s). Please update your code to avoid unnecessary renders.`);

feat: detect unnecessary render and warn user #468

Are you sure you want to change the base?

feat: detect unnecessary render and warn user #468

Conversation

guvenkaranfil commented Mar 14, 2024 • edited

Summary

Test plan

changeset-bot bot commented Mar 14, 2024 • edited

⚠️ No Changeset found

guvenkaranfil commented Mar 14, 2024

guvenkaranfil commented Mar 14, 2024 • edited

mdjastrzebski commented Mar 14, 2024

guvenkaranfil commented Mar 15, 2024

mdjastrzebski commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guvenkaranfil Mar 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guvenkaranfil commented Mar 26, 2024

mdjastrzebski commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdjastrzebski left a comment

Choose a reason for hiding this comment

guvenkaranfil commented Mar 26, 2024

guvenkaranfil commented Mar 29, 2024

guvenkaranfil commented Apr 1, 2024

guvenkaranfil commented Apr 11, 2024

mdjastrzebski commented Apr 14, 2024

guvenkaranfil commented May 2, 2024

mdjastrzebski commented May 2, 2024

guvenkaranfil commented May 2, 2024

mdjastrzebski commented May 3, 2024

guvenkaranfil commented Mar 14, 2024 •

edited

changeset-bot bot commented Mar 14, 2024 •

edited

guvenkaranfil commented Mar 14, 2024 •

edited

mdjastrzebski Mar 18, 2024 •

edited

mdjastrzebski Mar 18, 2024 •

edited

guvenkaranfil Mar 19, 2024 •

edited

mdjastrzebski Mar 26, 2024 •

edited