Skip to content

Setup a Macrobenchmark for RNTester#49486

Closed
janicduplessis wants to merge 9 commits intofacebook:mainfrom
janicduplessis:@janic/bench
Closed

Setup a Macrobenchmark for RNTester#49486
janicduplessis wants to merge 9 commits intofacebook:mainfrom
janicduplessis:@janic/bench

Conversation

@janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Feb 18, 2025

Summary:

Benchmark to test changes from #49449

Might be nice to have some version of this in the repo.

Changelog:

[INTERNAL] [ADDED] - Setup a Macrobenchmark for RNTester

Test Plan:

Methodology

Picked various JS file from websites (facebook, instagram) to artificially grow RN tester bundle somewhat realistically. The files are required lazily from a button press callback to simulate the code being included, but not executed, as it would be in a large app that uses lazy requires for the different screens.

I've also made the RN tester screens lazy so all their code is not loaded initially. This is more representative of real apps. Note this is implemented in a hacky way just for the purpose of this test. It would actually be nice to implement this properly.

The tests were made using low end device Samsung Galaxy A03s.

Compression ON with 10.5 mb bundle

Peak allocated memory

60.9 mb

ReactInstance.loadJSBundler

148.64 ms

Benchmark

timeToFullDisplayMs min 1,825.0, median 1,911.1, max 1,994.8
timeToInitialDisplayMs min 834.9, median 860.9, max 903.9

APK

Size: 22.9 mb
Download size: 14.5 mb

Compression OFF with 10.5 mb bundle

Peak allocated memory

51.5 mb

ReactInstance.loadJSBundler

946 us

Benchmark

timeToFullDisplayMs min 1,752.8, median 1,827.2, max 1,977.5
timeToInitialDisplayMs min 837.7, median 881.3, max 937.2

APK

Size: 28 mb
Download size: 14.5 mb

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Feb 18, 2025
@cortinico
Copy link
Contributor

Thanks for looking into those @janicduplessis 🙏

@janicduplessis
Copy link
Contributor Author

@cortinico Do you think we should try to merge this? I was thinking it could be useful to manually test perf improvements. Would be nice to run on ci too, but might be hard with the current setup since it seems to only make sense to run on physical device.

If so I can remove the changes to rn-tester and fake bundle size increase.

@cortinico
Copy link
Contributor

In general, I'd like to have such a thing merged. The problem is that this might slow down all the rn-tester gradle builds (because it adds a new build variant that we use like once in a while). So I'd rather keep it as a branch or PR around for when we need it

@janicduplessis
Copy link
Contributor Author

@cortinico Let me check the impact on gradle build time, if its negligible I think we can merge it.

@janicduplessis janicduplessis force-pushed the @janic/bench branch 2 times, most recently from a7c9517 to bb284dd Compare February 20, 2025 18:49
@cortinico
Copy link
Contributor

@cortinico Let me check the impact on gradle build time, if its negligible I think we can merge it.

I think that as we enabled Gradle Config Cache today, this is probably going to be small (and ok to maintain)

@janicduplessis
Copy link
Contributor Author

@cortinico Should we try to avoid having the benchmark variants built on CI or that's fine? I think it gets built because of this line here that includes all projects that use the gradle-plugin https://github.com/janicduplessis/react-native/blob/224fe93fecc17d98170888060b2b91872e1e2bf8/build.gradle.kts#L90.

@cortinico
Copy link
Contributor

@cortinico Should we try to avoid having the benchmark variants built on CI or that's fine? I think it gets built because of this line here that includes all projects that use the gradle-plugin janicduplessis/react-native@224fe93/build.gradle.kts#L90.

We'll have to do something like this: https://stackoverflow.com/a/34337260
and filter everything benchmark related till we actually need it (so we never build it)

@janicduplessis
Copy link
Contributor Author

Turns out we can just remove the benchmark variant and it still works fine using the hermesRelease variant instead.

@janicduplessis janicduplessis marked this pull request as ready for review February 21, 2025 00:35
@janicduplessis
Copy link
Contributor Author

From looking at ci logs it looks like the benchmark project does get built, but takes around 2 seconds so should be fine.

@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 21, 2025
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up @janicduplessis

@janicduplessis
Copy link
Contributor Author

I addressed the comments, once the PR to bump appcompat version lands I will rebase this and should be good to go!

facebook-github-bot pushed a commit that referenced this pull request Feb 21, 2025
Summary:
Update androidx app compat to the latest version.

This is needed as part of #49486 to have access to `fullyDrawnReporter`.

## Changelog:

[ANDROID] [CHANGED] - Update androidx app compat to 1.7.0

Pull Request resolved: #49594

Test Plan: Tested in RN tester that it builds fine and works properly.

Reviewed By: Abbondanzo

Differential Revision: D69988202

Pulled By: cortinico

fbshipit-source-id: 0329aa84a76327db535ddba8acf059ebbf1dbdfc
@janicduplessis
Copy link
Contributor Author

Rebased

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

@janicduplessis can we make sure the JS tests are green also?

cortinico and others added 2 commits February 26, 2025 09:51
Summary:
Tests have been executed with Android Studio:

**Startup**
```
# BEFORE

timeToInitialDisplayMs   min 216.9,   median 222.9,   max 245.3
Traces: Iteration 0 1 2 3 4 5 6 7 8 9

# AFTER

timeToInitialDisplayMs   min 213.8,   median 220.4,   max 237.9
Traces: Iteration 0 1 2 3 4 5 6 7 8 9
```

**APK size**
```
-rw-r--r--    1 ncor  staff  20087367 Feb 17 17:37 before.apk
-rw-r--r--    1 ncor  staff  20087399 Feb 17 18:43 after.apk
```

Changelog:
[Internal] [Changed] -

Differential Revision: D69753053
@janicduplessis
Copy link
Contributor Author

Fixed!

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@janicduplessis
Copy link
Contributor Author

@cortinico ReportFullyDrawnView actually doesn't need to wrap the app, I moved it and hopefully that solves the issue.

@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@cortinico merged this pull request in b614c96.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 4, 2025
@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in b614c96

When will my fix make it into a release? | How to file a pick request?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants