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

feat: detect unnecessary render and warn user #468

Merged
merged 22 commits into from
Jun 14, 2024

Conversation

guvenkaranfil
Copy link
Contributor

@guvenkaranfil guvenkaranfil commented Mar 14, 2024

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

Screenshot 2024-03-14 at 04 42 53

Copy link

changeset-bot bot commented Mar 14, 2024

🦋 Changeset detected

Latest commit: 6c6f88c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
reassure Minor
@callstack/reassure-compare Minor
@callstack/reassure-measure Minor
@callstack/reassure-cli Minor
@callstack/reassure-danger Minor
@callstack/reassure-logger Minor

Not sure what this means? Click here to learn what changesets are.

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

@guvenkaranfil
Copy link
Contributor Author

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
Copy link
Contributor Author

guvenkaranfil commented Mar 14, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

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

The important factors are:

  1. 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.
  2. The first run is ideal here, as it will usually be discarded (warmupRuns: 1 is the default setup).

@guvenkaranfil
Copy link
Contributor Author

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

@mdjastrzebski
Copy link
Member

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

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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.

Screenshot 2024-03-26 at 18 18 44

@guvenkaranfil
Copy link
Contributor Author

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

Screenshot 2024-03-29 at 06 19 13

@guvenkaranfil
Copy link
Contributor Author

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

@mdjastrzebski
Copy link
Member

@brandon-gs I am still polishing the UX & implementation around it. Probably still need around a week or two. BTW this feature would be released as part of 1.0.0 (perhaps as RC release).

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

LGTM

@mdjastrzebski
Copy link
Member

Performance Comparison Report

  • Current: feat/detectUnnecessaryRender (5ed8244205597933896d75d1ded089b5ff717121) - 2024-06-14 12:51:50Z
  • Baseline: feat/detectUnnecessaryRender (5494281) - 2024-06-13 22:19:48Z

Significant Changes To Duration

Name Type Duration Count
fib 32 function 79.9 ms → 89.1 ms (+9.2 ms, +11.5%) 🔴 1 → 1
fib 31 function 49.1 ms → 54.1 ms (+5.0 ms, +10.2%) 🔴 1 → 1
Show details
Name Type Duration Count
fib 32 function Baseline
Mean: 79.9 ms
Stdev: 0.2 ms (0.2%)
Runs: 79.7 79.8 79.9 79.9 79.8 79.9 80.1 79.9 80.3 79.6

Current
Mean: 89.1 ms
Stdev: 24.5 ms (27.5%)
Runs: 80.8 80.4 80.5 158.5 85.6 80.9 80.1 79.4 79.1 85.6
Baseline
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1

Current
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1
fib 31 function Baseline
Mean: 49.1 ms
Stdev: 0.3 ms (0.6%)
Runs: 49.6 49.2 49.0 49.2 48.8 48.7 49.1 49.5 48.8 49.2

Current
Mean: 54.1 ms
Stdev: 11.3 ms (20.9%)
Runs: 50.0 49.9 50.0 50.0 50.0 50.0 49.9 86.1 53.9 51.2
Baseline
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1

Current
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1

Meaningless Changes To Duration

Show entries
Name Type Duration Count
Async Component render 98.6 ms → 93.8 ms (-4.8 ms, -4.9%) 7 → 7
fib 30 function 30.2 ms → 32.5 ms (+2.3 ms, +7.6%) 1 → 1
Other Component 10 render 60.2 ms → 61.8 ms (+1.6 ms, +2.7%) 4 → 4
Other Component 10 legacy scenario render 61.6 ms → 57.8 ms (-3.8 ms, -6.2%) 4 → 4
Other Component 20 render 59.6 ms → 58.6 ms (-1.0 ms, -1.7%) 4 → 4
RedundantUpdates render 0.4 ms → 1.0 ms (+0.6 ms, +150.0%) 🔴 2 → 2
Show details
Name Type Duration Count
Async Component render Baseline
Mean: 98.6 ms
Stdev: 7.1 ms (7.2%)
Runs: 97 91 109 93 100 89 98 111 98 100

Current
Mean: 93.8 ms
Stdev: 3.9 ms (4.2%)
Runs: 95 94 93 99 100 94 89 94 87 93
Baseline
Mean: 7
Stdev: 0 (0.0%)
Runs: 7 7 7 7 7 7 7 7 7 7

Current
Mean: 7
Stdev: 0 (0.0%)
Runs: 7 7 7 7 7 7 7 7 7 7
fib 30 function Baseline
Mean: 30.2 ms
Stdev: 0.1 ms (0.3%)
Runs: 30.2 30.2 30.3 30.3 30.3 30.3 30.2 30.1 30.3 30.2

Current
Mean: 32.5 ms
Stdev: 3.2 ms (9.8%)
Runs: 30.8 30.7 30.8 30.8 30.4 39.8 36.6 31.1 31.1 33.1
Baseline
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1

Current
Mean: 1
Stdev: 0 (0.0%)
Runs: 1 1 1 1 1 1 1 1 1 1
Other Component 10 render Baseline
Mean: 60.2 ms
Stdev: 3.9 ms (6.5%)
Runs: 57 62 60 62 60 56 56 57 64 68

Current
Mean: 61.8 ms
Stdev: 3.2 ms (5.2%)
Runs: 56 64 65 65 61 59 60 60 62 66
Baseline
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4

Current
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4
Other Component 10 legacy scenario render Baseline
Mean: 61.6 ms
Stdev: 4.1 ms (6.6%)
Runs: 58 62 64 61 59 62 59 60 59 72

Current
Mean: 57.8 ms
Stdev: 3.4 ms (5.9%)
Runs: 58 60 58 56 53 57 54 56 63 63
Baseline
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4

Current
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4
Other Component 20 render Baseline
Mean: 59.6 ms
Stdev: 6.2 ms (10.4%)
Runs: 62 62 61 54 54 63 61 54 55 69 55 56 60 67 78 54 56 60 57 55

Current
Mean: 58.6 ms
Stdev: 4.9 ms (8.4%)
Runs: 61 53 56 58 57 57 54 57 51 62 57 63 58 61 75 59 59 55 60 60
Baseline
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4

Current
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4 4
RedundantUpdates render Baseline
Mean: 0.4 ms
Stdev: 0.5 ms (129.1%)
Runs: 0 1 0 1 1 0 1 0 0 0

Current
Mean: 1.0 ms
Stdev: 0.9 ms (94.3%)
Runs: 2 1 0 2 2 0 0 2 1 0
Baseline
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2

Current
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2

Render Count Changes

There are no entries

Render Issues

Name Initial Updates Redundant Updates
Other Component 10 1 🔴 -
Other Component 10 legacy scenario 1 🔴 -
Other Component 20 1 🔴 -
RedundantUpdates - 1 (1) 🔴
Async Component 1 🔴 -
InitialRenders 1 1 🔴 -
InitialRenders 3 3 🔴 -
ManyRenderIssues 2 🔴 2 (3, 4) 🔴

Added Scenarios

Name Type Duration Count
InitialRenders 1 render 1.1 ms 2
InitialRenders 3 render 1.4 ms 4
ManyRenderIssues render 1.8 ms 5
Show details
Name Type Duration Count
InitialRenders 1 render Current
Mean: 1.1 ms
Stdev: 0.3 ms (28.7%)
Runs: 2 1 1 1 1 1 1 1 1 1
Current
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2
InitialRenders 3 render Current
Mean: 1.4 ms
Stdev: 0.7 ms (49.9%)
Runs: 2 2 1 0 1 2 2 2 1 1
Current
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4
ManyRenderIssues render Current
Mean: 1.8 ms
Stdev: 0.8 ms (43.8%)
Runs: 1 1 1 3 2 2 3 1 2 2
Current
Mean: 5
Stdev: 0 (0.0%)
Runs: 5 5 5 5 5 5 5 5 5 5

Removed Scenarios

Name Type Duration Count
RedundantInitialRenders 1 render 0.7 ms 2
RedundantInitialRenders 3 render 0.6 ms 4
Show details
Name Type Duration Count
RedundantInitialRenders 1 render Baseline
Mean: 0.7 ms
Stdev: 0.5 ms (69.0%)
Runs: 1 1 0 0 1 1 1 0 1 1
Baseline
Mean: 2
Stdev: 0 (0.0%)
Runs: 2 2 2 2 2 2 2 2 2 2
RedundantInitialRenders 3 render Baseline
Mean: 0.6 ms
Stdev: 0.5 ms (86.1%)
Runs: 0 1 1 0 0 0 1 1 1 1
Baseline
Mean: 4
Stdev: 0 (0.0%)
Runs: 4 4 4 4 4 4 4 4 4 4

@mdjastrzebski
Copy link
Member

Console version for the report:
image

@mdjastrzebski mdjastrzebski merged commit 04be1d4 into callstack:v1 Jun 14, 2024
1 check passed
@mdjastrzebski
Copy link
Member

🎉 This PR has been released in v1.0.0-rc.5 🎊

mdjastrzebski added a commit that referenced this pull request Jun 16, 2024
* refactor: rename `meassurePerformance` to `measureRenders` (#433)

* chore: v1.0.0-beta

* docs: update changelogs

* docs: migration guide

* chore: update snapshots

* chore: update node ver on GH

* chore: v1.0.0-rc

* chore: v1.0.0-rc.1

* chore: reorganize repo deps (#477)

* refactor: migrate deps

* chore: dedup eslint

* chore: fix eslint deps

* chore: yarn berry (#478)

* chore: yarn berry (wip)

chore: forward reassure cli in the main package

chore: remove --force

chore: fix ci

chore: fix ci

chore: fix typecheck

chore: fix danger

chore: fix danger

chore: fix ci

chore: fix danger

refactor: tweaks

* chore: workspace deps

* chore: add changeset

* chore: v1.0.0-rc.2

* chore: fix versions

* chore: fix version deps between packages

* chore: v1.0.0-rc.3

* fix: iso date milliseconds regex (#481)

* chore: check deps consistency (#482)

* chore: pdate deps 2024-05-09 (#484)

* chore: update deps

* chore: fix lint

* chore: update danger (#483)

chore: update danger.js

* chore: upgrade RN / test app (#485)

* chore: upgrade turbo

* chore: update React Native / test app

* chore: fix wasm error

* chore: updata other deps (#486)

* chore: update other deps

* chore: update peer deps

* chore: update deps

* feat: enable wasm by default (#488)

* chore: tweak workflow (#490)

* chore: tweak workflow

* chore: update docs

* chore: export types (#489)

* chore: export types

* chore: gen yarn.lock

* chore: tweaks docs

* refactor: rename types

* chore: add docs

* chore: clean scripts (#491)

* chore: clean scripts

* chore: tweak turbo

* chore: add missing changeset

* chore: v1.0.0-beta.4

* chore: regenerate yarn.lock

* feat: detect unnecessary render and warn user (#468)

* feat: detect unnecessary render and warn user

feat: capture json representations in an array to compare after for unnecessary rendering

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

fix: save json representation instead of string for rendered component state and compare changes between states using dfs

fix: test name

fix: update comparasion function and save compare results into output.json and show comparation results in the end of the test

fix: update interface and variable names based on pr recommendatations

chore: rebase to v1

* refactor: improve output formatting

* refactor: fix typo

* refactor: remove unnecessary warning

* refactor: improve testing

* refactor: improve report criteria

* refactor: improve naming

* refactor: improve code structure & tests

* refactor: tweaks

* feat: improve markdown output

* refactor: custom tree comparer

* refactor: clean up code

* refactor: update JSON structure

* refactor: use "initial update count" naming

* chore: fix lint

* chore: improve tests

* refactor: tweaks

* docs: update

* refactor: self code review

* docs: tweaks

* chore: add changeset

* refactor: final tweaks

---------

Co-authored-by: Guven Karanfil <[email protected]>
Co-authored-by: Maciej Jastrzebski <[email protected]>

* chore: v1.0.0-rc.5

* chore: fix yarn.lock

* chore: fix reassure-tests.sh

---------

Co-authored-by: Güven Karanfil <[email protected]>
Co-authored-by: Guven Karanfil <[email protected]>
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