[native] Fix Android CI
ClosedPublic
Actions

Authored by ashoat on Apr 22 2023, 1:53 PM.

Details

Reviewers

atul
• jon
bartek
tomek

Commits

rCOMMa808e6be7564: [native] Fix Android CI

Summary

Android Buildkite CI is consistently showing this error now (example):

Unrecognized VM option 'MaxPermSize=4096m'

Apparently this option has been deprecated.

This option was removed in the React Native template's gradle.properties starting in 0.69, but I neglected to remove it during my most recent upgrade since I assumed our customization should be kept based on the contents of 24dd5bd2b9de7ffff06db6aa0d755ebfcc2730d4.

Given that the option is now causing CI to fail, and given that the template's config seems to include an updated heap size now, I figure we can realign our gradle.properties back to match what is in the React Native template.

Test Plan

See if this diff passes Buildkite CI

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Apr 22 2023, 1:53 PM

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 22 2023, 2:16 PM

Harbormaster failed remote builds in B18670: Diff 25570!

Also update .buildkite/android.yml

ashoat edited the summary of this revision. (Show Details)Apr 22 2023, 2:32 PM

ashoat added inline comments.

.buildkite/android.yml
11 ↗	(On Diff #25571)	Wasn't sure what to set `MaxMetaspaceSize` to... opted to keep it as 1/4 of `-Xmx` value

Harbormaster completed remote builds in B18671: Diff 25571.Apr 22 2023, 2:50 PM

ashoat requested review of this revision.Apr 22 2023, 2:50 PM

atul accepted this revision.Apr 22 2023, 6:13 PM

atul added inline comments.

.buildkite/android.yml
11 ↗	(On Diff #25571)	I think we can keep it the same as `-Xmx`. TBH I'm not sure we even really need to specify `MaxMetaspaceSize` and can let the JVM handle it. From what I'm reading `MaxMetaspaceSize` replaces `MaxPermSize` with some differences between memory management/garbage collection or whatever... I don't think it would hurt to leave it set as 32g? We're setting an upper limit here but I'm pretty sure JVM will have lower "realized" limit if necessary. This is only for CI machines so responsiveness of computer isn't a factor like it is for dev machines

This revision is now accepted and ready to land.Apr 22 2023, 6:13 PM

atul added inline comments.Apr 22 2023, 6:52 PM

native/android/gradle.properties
13 ↗	(On Diff #25571)	I think we can just leave these values unset? Looked at OpenJDK code out of curiosity to see how they calculate max values if not set manually by `jvmargs` and found the following: (https://github.com/openjdk/jdk/blob/8d696aea9e1cefca97a760c017a5fde545facaa9/src/hotspot/share/runtime/arguments.cpp#L1568) TLDR `Arguments::set_heap_size()` sets `reasonable_max` based on various constants and available hardware. The key constant seems to be `MaxRAMPercentage` defined in `gc_globals.hpp`: Which by default is set to 25% of physical RAM, but can be overridden with `jvmargs`. EG on my machine it would set the maximum here to 8GB (32/4) instead of 4GB. This seems like a reasonable default and we don't have to worry about setting these values for the "lowest common denominator"? I think we can leave this unset and manually set `MaxRAMPercentage` to like 50-75% for CI?

atul attached a referenced file: F502509: 597b8d.png. (Show Details)Apr 22 2023, 6:52 PM

atul attached a referenced file: F502505: b86de9.png. (Show Details)

atul attached a referenced file: F502504: a4d9ca.png. (Show Details)

Going to land this as-is to fix up master ahead of the week starting

.buildkite/android.yml
11 ↗	(On Diff #25571)	Hmm, can I leave it to you to submit these changes? Makes sense that we should maybe remove these params, but I'm not really aware of why we initially introduced them, and I don't have a good intuition for how to set them
native/android/gradle.properties
13 ↗	(On Diff #25571)	I'd rather leave it matching the React Native template... I'm not sure if there is some special consideration they have that we're not aware of