-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Changes from 10 commits
9f1f69f
2261661
b345460
2035bae
4f13d3c
5e6f307
1898c85
24bdecc
54c6548
fba17fc
87c3dd5
f71a522
51e8996
8d0fbd6
8b58598
d808141
d646941
ae56422
1b2a031
de4f213
b130582
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,97 @@ 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) and email [email protected] to request access to our Slack channel! | ||
|
|
||
| 👉 **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. | ||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 2. Fill out the details strictly following the guide. | ||
| 3. Post it in `#expensify-open-source` with the title `[Performance Proposal] <Component_Name>`. | ||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ___ | ||
|
|
||
| ``` | ||
| ## 1. Component and Flow Description | ||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| **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 Flamecharts | ||
| - [ ] Hermes / Release Profiler traces | ||
| - [ ] Sentry (If you have access) | ||
|
|
||
| ## 3. Before/After Metrics | ||
| *Please fill out the table below. If a metric is not applicable, write N/A.* | ||
|
|
||
| | Metric | Before | After | Improvement | | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| | :--- | :--- | :--- | :--- | | ||
| | **Render Count** | | | | | ||
| | **Execution Time** | | | | | ||
| | **Perceived Latency** | | | | | ||
|
||
|
|
||
| * **Device Used:** (e.g. iPhone 13, Pixel 6, Chrome on M1 Mac) | ||
| * **Evidence:** *(Attach screenshots of the profiler or logs for both Before and After below this section)* | ||
|
|
||
| ## 4. Prerequisites & Eligibility | ||
| *To ensure proposals are measurable and based on realistic scenarios, you must meet the following criteria:* | ||
|
|
||
| - [ ] **Experience:** I have at least **1 merged PR** in the App repository. | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] **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
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - [ ] **Thresholds:** My proposal meets **at least one** of the following: | ||
| - [ ] > 20% reduction in Render Count | ||
| - [ ] > 20% reduction in Execution Time | ||
| - [ ] > 100ms reduction in Perceived Latency | ||
luacmartins marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ## 5. Pattern Detection & Prevention | ||
| *Can we prevent this from happening again?* | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] **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.
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. I def agree, but how about we release the current version, get feedback, and then update? 🙏 I just want to not wait too too long - we'll start with some internal-ish folks (C+) i believe so it's fine if it's not perfect to start, but at least let's get started 🙏
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. Sounds good |
||
|
|
||
| --- | ||
|
|
||
| ### 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, tag `@Expensify/performance-reviewers`. | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 3. **Approval:** **2 Internal Engineers** must approve before a GH issue is created. | ||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We handle this via a google form now. Easiest to just delete this text cuz it's already in CONTRIBUTING.md . If you want to keep, update to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh ok well maybe I can just keep the first part (check out our contributing guidelines) but remove the part about requesting slack channel access? I think it's always a good reminder & good practice to link to our main CONTRIBUTING docs - but we don't have to explain any processes here that are found there, as you said!