-
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 2 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,89 @@ 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 perceived user 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.** | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ___ | ||
|
|
||
| ### 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.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ___ | ||
|
|
||
| ## 1. 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)). | ||
|
||
| - [ ] **Thresholds:** My proposal meets **at least one** of the following: | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] > 20% reduction in Render Count | ||
| - [ ] > 20% reduction in Execution Time | ||
| - [ ] > 100ms reduction in Perceived Latency | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ## 2. Component and Flow Description | ||
| - **Component/Flow:** Describe the specific UI component or user flow being optimized. | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - **Preconditions:** List any specific setup required before reproducing the steps (e.g., "Workspace must have chat history"). | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - **Reproduction Steps:** Provide a numbered list of steps to reproduce the performance issue (similar to a QA test case). | ||
|
|
||
| ## 3. 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) | ||
|
|
||
| ## 4. 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)* | ||
|
|
||
| ## 5. Pattern Detection & Prevention | ||
| *Can we prevent this from happening again?* | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] **ESLint Rule:** I have proposed a rule to prevent this anti-pattern. | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] **Shared Refactor:** This fixes a shared utility/component (e.g., `Avatar.js`) used across the app. | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] **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 generally be invisible to the user.* | ||
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| - [ ] This change preserves existing UX (No visual/behavioral changes). | ||
| - [ ] This change alters UX (Description: _________________). | ||
|
|
||
| --- | ||
|
|
||
| ### 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. | ||
Uh oh!
There was an error while loading. Please reload this page.