-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[HOLD discussion] Add section for Proposing Performance Improvements #78757
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
base: main
Are you sure you want to change the base?
Changes from all commits
9f1f69f
2261661
b345460
2035bae
4f13d3c
5e6f307
1898c85
24bdecc
54c6548
fba17fc
87c3dd5
f71a522
51e8996
8d0fbd6
8b58598
d808141
d646941
ae56422
1b2a031
de4f213
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -361,4 +361,98 @@ Examples: | |||||||||
| - [Remove shouldAdjustScrollView to avoid heavy rerender](https://github.com/Expensify/App/pull/66849) - removes hooks that were called only for Safari logic slowing down the `ReportScreen.tsx` | ||||||||||
| - [PopoverWithMeasuredContent optimization for mobile](https://github.com/Expensify/App/pull/68223) - returns early to avoid unnecessary calculations | ||||||||||
| - [Reduce confirm modal initial render count](https://github.com/Expensify/App/pull/67518) - returns early to reduce first load cost | ||||||||||
| - [Do not render PopoverMenu until it gets opened](https://github.com/Expensify/App/pull/67877) - adds a wrapper to control if `PopoverMenu` should be rendered | ||||||||||
| - [Do not render PopoverMenu until it gets opened](https://github.com/Expensify/App/pull/67877) - adds a wrapper to control if `PopoverMenu` should be rendered | ||||||||||
|
|
||||||||||
| # Proposing Performance Improvements | ||||||||||
|
|
||||||||||
| We are actively looking for contributions that improve the performance of the App, specifically regarding unnecessary re-renders, slow method executions, and user perceived latency. | ||||||||||
|
|
||||||||||
| If you haven't already, check out our [Contributing Guidelines](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md). | ||||||||||
|
|
||||||||||
| 👉 **Before posting the proposal, please read through this whole process for important context and instructions.** Proposals that do not follow these guidelines cannot be accepted. | ||||||||||
|
|
||||||||||
| ___ | ||||||||||
|
|
||||||||||
| ### Instructions for Submission | ||||||||||
| 1. Copy the template below. | ||||||||||
| 2. Fill out the details strictly following the guide. | ||||||||||
| 3. Post it in `#expensify-open-source` with the title `[Performance Proposal] <Component_Name>`. | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Title should be included as the header of the Slack message content |
||||||||||
|
|
||||||||||
| ___ | ||||||||||
|
|
||||||||||
| ``` | ||||||||||
| ## 1. Component and Flow Description | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Example |
||||||||||
|
|
||||||||||
| **Component/Flow:** Describe the specific UI component or user flow being optimized. | ||||||||||
| - [Add details here] | ||||||||||
|
|
||||||||||
| **Preconditions:** List any specific setup required before reproducing the steps (e.g., "Workspace must have chat history"). | ||||||||||
| - [Add details here] | ||||||||||
|
|
||||||||||
| **Reproduction Steps:** Provide a numbered list of steps to reproduce the performance issue (similar to a QA test case). | ||||||||||
| - [Add details here] | ||||||||||
|
|
||||||||||
| ## 2. Required Tools | ||||||||||
| *I have verified these metrics using (check all that apply):* | ||||||||||
| - [ ] React DevTools Profiler | ||||||||||
| - [ ] Chrome Performance Tab | ||||||||||
| - [ ] JS Flame charts | ||||||||||
| - [ ] Hermes / Release Profiler traces | ||||||||||
| - [ ] Sentry (If you have access) | ||||||||||
|
|
||||||||||
| ## 3. Before/After Metrics | ||||||||||
| *Please fill out the metrics below. If a metric is not applicable, write N/A.* | ||||||||||
|
|
||||||||||
| *Perceived Latency:* | ||||||||||
| - Before: | ||||||||||
| - After: | ||||||||||
| - Improvement: | ||||||||||
|
|
||||||||||
| *Device Used:* (e.g. iPhone 13, Pixel 6, Chrome on M1 Mac) - Note: Don't use CPU throttling for these measurements! | ||||||||||
| - Device CPU: ___ | ||||||||||
| - Device RAM: ___ | ||||||||||
| *Evidence:* (Attach screenshots of the profiler or logs for both Before and After below this section)* | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these attachments/logs be attached inside the thread of the Slack message? If yes I think we should be clear about it. For example, "below this section" won't work in Slack like that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean clear that it should be the thread and not the main post? It looks clear to me that is should be attached |
||||||||||
|
|
||||||||||
| ## 4. Prerequisites & Eligibility | ||||||||||
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||||||||||
|
|
||||||||||
| - [ ] **Test Environment:** I tested on a high-traffic account (instructions to create this [here](https://github.com/Expensify/App/blob/main/contributingGuides/CONTRIBUTING.md#high-traffic-accounts)). | ||||||||||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we agreed to use a curated onyx state file obtained from the heavy applause account? https://expensify.slack.com/archives/C05LX9D6E07/p1765376829013879
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah in slack we left it there, but in this PR (here) we discussed and thought it would be better to go with a live account
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think we are going abck and forth there but we also seen that offline, lots of events get hidden the performance offline is not exactly what we get when user is online, so we would like to use account that is online preferably |
||||||||||
| - [ ] **Thresholds:** My proposal reduces Perceived Latency by at least 100ms | ||||||||||
|
|
||||||||||
| ## 5. Pattern Detection & Prevention | ||||||||||
| *Is the code logic being optimized something that should be prevented from being added to the app in the future (e.g., via an ESLint rule)?* | ||||||||||
| - [ ] Yes (Proposal: _________________) | ||||||||||
| - [ ] No (It's a valid pattern, just unoptimized here) | ||||||||||
|
|
||||||||||
| *Other* | ||||||||||
| - [ ] **App-wide Audit:** I have checked for other places in the app that have this same performance problem and fixed them. | ||||||||||
| - [ ] **Shared Refactor:** This fixes a shared utility/component (e.g., `Avatar.ts`) used across the app. | ||||||||||
| - [ ] **Localized Fix:** This only affects this specific view. | ||||||||||
|
|
||||||||||
|
|
||||||||||
| ## 6. Automated Tests & QA | ||||||||||
| *Tests are required by default. If you cannot add them, explain why.* | ||||||||||
| - [ ] **Unit Tests:** Added to prevent regression. | ||||||||||
| - [ ] **Reassure Tests:** Added (Required for execution time improvements). | ||||||||||
| - [ ] **Exception:** I cannot add automated tests because: _________________ | ||||||||||
| - [ ] **Manual Verification:** I have included manual verification steps (Required). | ||||||||||
|
|
||||||||||
| ## 7. Other Considerations & UX Risks | ||||||||||
| *Performance improvements should not change user experience and product design.* | ||||||||||
| - [ ] This change preserves existing UX (No visual/behavioral changes). | ||||||||||
| - [ ] This change alters UX (Description: _________________). | ||||||||||
| ``` | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
|
|
||||||||||
| --- | ||||||||||
|
|
||||||||||
| ### Compensation | ||||||||||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
| * **Bounty:** Accepted and merged performance improvements are eligible for a flat **$250 bounty**. | ||||||||||
| * **Scope:** We prefer smaller, atomic PRs. However, if multiple proposals are submitted for closely related logic that could have been one PR, we reserve the right to consolidate them. | ||||||||||
|
|
||||||||||
| ___ | ||||||||||
|
|
||||||||||
| ### Review Process | ||||||||||
| 1. **Peer Review:** Wait for **2 Expert Contributors** to approve your proposal. | ||||||||||
| 2. **Internal Review:** Once approved by experts, comment `Proposal ready for final review - cc: perf-review` in slack to notify Internal Engineers that the proposal is ready a final review. | ||||||||||
| - Note: Internal Engineers can set up notifications for `perf-review` keyword as mentioned in [this Internal SO](https://stackoverflowteams.com/c/expensify/questions/23081/23082#23082). | ||||||||||
| 3. **Approval:** **2 Internal Engineers** must approve before a GH issue is created. | ||||||||||


Uh oh!
There was an error while loading. Please reload this page.