Repository navigation
[Task 1/5] [No QA] Add native PGO build and benchmark tooling - #97808
chrispader wants to merge 60 commits into
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@bernhardoj Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f329c841
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@chrispader what is the order in which the PRs need to be merged? |
Posted the order here: |
chrispader
changed the title
|
@chrispader based on the order, I think this is still on hold? |
@bernhardoj no this should be merged together with https://github.com/Expensify/Mobile-Expensify/pull/14046. I've updated the merge order in the issue accordingly, to make it easier to understand |
| + echo "EXPENSIFY_PGO_PROFILE is required when EXPENSIFY_PGO_MODE=use" >&2 | ||
| + exit 1 | ||
| + fi | ||
| + pgo_compile_flags="-fprofile-use=${EXPENSIFY_PGO_PROFILE}" |
There was a problem hiding this comment.
Is it expected that the pgo_linker_flags is not set for "use" mode here?
| dispatch_once(&onceToken, ^{ | ||
| CFNotificationCenterRef notificationCenter = CFNotificationCenterGetDarwinNotifyCenter(); | ||
| CFNotificationCenterAddObserver(notificationCenter, NULL, HandlePGONotification, PGO_WRITE_NOTIFICATION, NULL, CFNotificationSuspensionBehaviorDeliverImmediately); | ||
| CFNotificationCenterAddObserver(notificationCenter, NULL, HandlePGONotification, PGO_CLEAR_NOTIFICATION, NULL, CFNotificationSuspensionBehaviorDeliverImmediately); |
There was a problem hiding this comment.
Notifications are registered even when EXPENSIFY_PGO_GENERATE is disabled.
| } | ||
| if (result.status !== 0) { | ||
| const stderr = result.stderr.trim(); | ||
| fail(stderr.length > 0 ? stderr : `${command} exited with status ${result.status ?? 'unknown'}.`); |
There was a problem hiding this comment.
If stderr is truthy, we won't know what command fails.
|
@chrispader Can I test iOS without a release signing certificate? |
|
I get this error when running Claude said:
|
| }); | ||
|
|
||
| it('reads the bootstrapped release application ID', () => { | ||
| // Given |
There was a problem hiding this comment.
Some tests missing Given When Then explanation
|
Startup benchmark results on Android:
|
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
(Vit's agent) Full pass over this PR and Mobile-Expensify#14046. The checks that fail closed are good: the iOS format marker, the all-4 Android profile check, no merge of partial journeys, and no automated sign-in. Line comments are inline. These blockers don't map to one line:
- No native build has run. This PR comes from a fork, so
verifyHybridApp.ymlskipped both HybridApp builds. The RN patch, the CMake changes, andRCTBootSplash.mmship in production builds, so we need an AdHoc build on both platforms before merge. - Merge order. Mobile-Expensify#14046 calls
+[RCTBootSplash initializePGOProfileCollection], which only exists after this PR. If it lands first, the iOS HybridApp fails to compile. Order: this PR, then Mobile-Expensify#14046, then the submodule bump. - iOS production gate. +1 to bernhardoj's comment. Mobile-Expensify#14046 calls this on every launch, and any process on the device can post Darwin notifications. Please wrap the observer registration in
#if EXPENSIFY_PGO_GENERATE. - Open threads. The proposal on the tracking issue still needs approval. bernhardoj's build failure, iOS signing question, and inline comments have no replies yet.
| '-PreactNativeArchitectures=arm64-v8a', | ||
| `-PpgoMode=${pgoModes[kind]}`, | ||
| '-x', | ||
| 'fullstoryRelease', |
There was a problem hiding this comment.
(Vit's agent) build-instrumented fails on a clean checkout with createReleaseApkListingFileRedirect (bernhardoj's report). Skipping that task too fixes it:
| 'fullstoryRelease', | |
| 'fullstoryRelease', | |
| '-x', | |
| 'createReleaseApkListingFileRedirect', |
|
|
||
| <receiver | ||
| android:name=".PgoProfileReceiver" | ||
| android:exported="true"> |
There was a problem hiding this comment.
(Vit's agent) This receiver ships in production with no permission, so any app on the device can send it. One line locks it to adb: the shell user holds DUMP, and normal apps can't get it. Mobile-Expensify#14046 needs the same change.
| android:exported="true"> | |
| android:exported="true" | |
| android:permission="android.permission.DUMP"> |
| } | ||
|
|
||
| System.loadLibrary("ExpensifyNitroUtils"); | ||
| return writeProfiles(context.getCacheDir().getAbsolutePath()); |
There was a problem hiding this comment.
(Vit's agent) The PGO scripts only target HybridApp (org.me.mobiexpensifyg), so this standalone copy never runs. It also writes to getCacheDir(), which adb can't pull from a release build. The Mobile-Expensify copy uses getExternalCacheDir(). Can we drop the android/app changes, or make them match?
|
|
||
| + (void)initWithStoryboard:(NSString * _Nonnull)storyboardName | ||
| rootView:(UIView * _Nullable)rootView; | ||
| + (void)initializePGOProfileCollection; |
There was a problem hiding this comment.
(Vit's agent) Nit: this puts about 100 lines of profiling code in the boot splash module. A small ExpensifyPGO.mm would keep it separate and make the #if EXPENSIFY_PGO_GENERATE gate simpler.
| OUTPUT_VARIABLE CLANG_RESOURCE_DIR | ||
| OUTPUT_STRIP_TRAILING_WHITESPACE | ||
| ) | ||
| set(PGO_RUNTIME "${CLANG_RESOURCE_DIR}/lib/linux/libclang_rt.profile-aarch64-android.a") |
There was a problem hiding this comment.
(Vit's agent) This hardcodes the aarch64 runtime, and so do the two copies in the RN patch. A generate build with the default ABIs will fail to link. Could Gradle reject any pgoMode other than off unless reactNativeArchitectures=arm64-v8a?
| rmSync(directory, {recursive: true, force: true}); | ||
| } | ||
| } | ||
| run('/usr/bin/env', [...sourceBuildEnvironment(mode, profilePath), 'bundle', 'exec', 'pod', 'install', '--silent'], iosDirectory); |
There was a problem hiding this comment.
(Vit's agent) This switches Mobile-Expensify's Pods to source builds, and nothing switches them back after the run. Please restore them, or call it out in the README.
| ]; | ||
| } | ||
| if (kind === 'optimized') { | ||
| const clangProfileFlags = `-fprofile-use=${profilePath} -Wno-error=profile-instr-unprofiled`; |
There was a problem hiding this comment.
(Vit's agent) Xcode splits OTHER_CFLAGS on spaces, so a profile path with a space breaks this flag (and the Swift flag below). pgoIOSRock.test.ts only checks Rock's parser, so it doesn't catch this. Please quote the path.
| if (device.platform === 'android') { | ||
| device.fill('composer', `${token} draft`); | ||
| } | ||
| device.fill('composer', `${token}. Answered by Codex, instructed by Chris.`); |
There was a problem hiding this comment.
(Vit's agent) Every journey run posts this text into a real chat. Please drop the personal and tool names:
| device.fill('composer', `${token}. Answered by Codex, instructed by Chris.`); | |
| device.fill('composer', `${token}. Automated PGO journey message.`); |
| function assertSignedIn(nodes: JourneyNode[]): void { | ||
| const signInLabels = new Set(['Sign in', 'Email or phone number', 'Enter your email or phone number', 'Enter your magic code']); | ||
| if (nodes.some((node) => ['username', 'validateCode'].includes(node.identifier) || signInLabels.has(normalizeLabel(node.label)))) { | ||
| throw new Error('SIGN_IN_REQUIRED: Ask Chris to sign into the approved heavy account, then restart the journey. Automated sign-in is disabled.'); |
There was a problem hiding this comment.
(Vit's agent) This error names one person, and so do journeyWorkload.ts:74, journey.ts:44, and the README. Other engineers and CI will run this, so please say "the account owner" instead.
|
|
||
| Start with three repetitions per platform. Use the recorded duration and resulting native counter coverage to adjust the workload; extra repetitions of the same data do not add new paths. Most activity here exercises Hermes, Fabric mounting, layout, text, native input, and scrolling. This trains the compiled native implementation used by JavaScript; it does not compile the application's JavaScript with LLVM. Avoid spending most of the run waiting on network responses or looping over trivial settings screens. | ||
|
|
||
| On September 23, 2026, the Android navigation-only run completed in 6 minutes 7 seconds. Two complete instrumented recordings passed in 7 minutes 1 second and 6 minutes 55 seconds, excluding preflight. Each sent two messages and exported four raw profiles; both recordings merged successfully, separately and together. These timings include automation overhead and the final account/preference check; they are not app performance measurements. The first recording's four profiles were also checked individually for nonzero native counters. |
There was a problem hiding this comment.
(Vit's agent) Nit: dated run logs go stale quickly. Could these move to the tracking issue?
This branch has not been deployed
5 participants
Native release builds currently lack a reproducible way to train and validate LLVM optimization profiles. This PR adds opt-in build, collection and benchmark tooling for Android and iOS, while leaving CI workflow activation to a separate PR.
Explanation of Change
The local tooling builds baseline, instrumented and optimized native apps, archives profiles and compares startup and journey measurements. The Android switches cover source-built React Native, Hermes and native utilities; the iOS switches cover the app and source-built CocoaPods targets.
The iOS corrections use compatible Clang, Swift and Hermes IR instrumentation, explicitly forward the Swift frontend flags, and build through Rock with quoted Xcode settings. Profiles carry the current ios-clang-swift-ir-v3 marker so earlier instrumentation formats must be recollected. The App dependency now points to the clean updated native PGO foundation instead of a revision without profiling hooks.
This PR changes no CI workflows. Separate evaluation tooling and report-only CI build on this foundation. The portable iOS corrections were exercised on a physical iPhone in a pinned precursor checkout; this branch has regression coverage but has not had a fresh native rebuild or an actual CI/store-export run. That local smoke run establishes no performance gain.
cc @mountiny @roryabraham
Fixed Issues
$ #98008
PROPOSAL: #98008
The tracking issue contains the proposal. Formal proposal approval remains pending.
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14046
Tests
Offline tests
QA Steps
This PR changes profiling and build infrastructure. QA cannot establish compiler instrumentation or profile consumption through the staging app UI. Native engineers must repeat the documented physical-device checks before enabling the separate CI workflow.
PR Author Checklist
I linked the correct issue in the
### Fixed Issuessection aboveI wrote clear testing steps that cover the changes made in this PR
TestssectionOffline stepssectionQA stepssectionI included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I verified any copy / text that was added to the app is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected)If new assets were added or existing ones were modified, I verified that:
npm run compress-svg)If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases)If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
Designlabel and/or tagged@Expensify/designso the design team can review the changes.If the PR adds or modifies the UI:
I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
If the
mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.I followed proper code patterns (see Reviewing the code)
I followed the guidelines as stated in the Review Guidelines
If a new CSS style is added I verified that:
StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Screenshots/Videos
No screenshots or videos are included. This change affects profiling and build tooling and adds no app UI.