[HOLD discussion] Add section for Proposing Performance Improvements #78757
Conversation
melvin-bot
bot
requested review from
Krishna2323
and removed request for
a team
|
@Krishna2323 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] |
contributingGuides/PERFORMANCE.md
Outdated
| *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. | ||
| - [ ] **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)). |
There was a problem hiding this comment.
Still deciding if we should go with a high traffic account, OR link to some cleaned onyx state for download
There was a problem hiding this comment.
I think having a real online account is better so it is closer to real-life user experience, however, I think we might need a bit more complex account than the high-traffic account. It does not have any transactions/ approvers etc as far as I know. Maybe we could beef it up with some script?
There was a problem hiding this comment.
I agree with both things Vit said.
We also want to be careful that this demo account doesn't become outdated as we release more and more functionality. Just something to keep in mind, don't need to solve it right now.
There was a problem hiding this comment.
Love the plan to use a "real online account" instead of purely importing onyx state, and yeah maybe a follow-up to getting this running will be to figure out how to beef up accounts for many-transactions / many-workspace-members / etc?
Beamanator
changed the title
contributingGuides/PERFORMANCE.md
Outdated
| *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. | ||
| - [ ] **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)). |
There was a problem hiding this comment.
I think having a real online account is better so it is closer to real-life user experience, however, I think we might need a bit more complex account than the high-traffic account. It does not have any transactions/ approvers etc as far as I know. Maybe we could beef it up with some script?
contributingGuides/PERFORMANCE.md
Outdated
| - [ ] **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 |
There was a problem hiding this comment.
I thought we agreed that with the execution time, it should be a percentage AND a minimum absolute value. Because we want to avoid someone improving something that takes 50ms. This way, they would improve it to 39ms, which would be valid. Eventually we should focus on these too, but right now, I think we should try to make sure the proposals aim for bigger performance improvements
There was a problem hiding this comment.
Hmm am I misunderstanding the convo in this thread?
I feel like I see "OR" a lot but maybe I'm not quite understanding what you're recommending?
There was a problem hiding this comment.
Yeah that thread seems to incline to simply OR, but I believe people would agree we dont want to look at short spans like 30ms and getting 20% shaved off that. I think we need to give this some boundary too (like min time of the span)
contributingGuides/PERFORMANCE.md
Outdated
| *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. | ||
| - [ ] **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)). |
There was a problem hiding this comment.
I agree with both things Vit said.
We also want to be careful that this demo account doesn't become outdated as we release more and more functionality. Just something to keep in mind, don't need to solve it right now.
| * **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?* |
There was a problem hiding this comment.
What is "this" referring to? I don't quite understand the question.
There was a problem hiding this comment.
It's referring to the previous state of "worse performance" - this is supposed to be generic so it can apply to all proposals, but maybe it's too unclear & therefore won't get any useful response 😅
There was a problem hiding this comment.
OK, I think this is covered by one of the other points, but basically... I don't think we should assume that the performance problem being fix was bad, or an anti-pattern. A lot of times, it's just because it wasn't pre-optimized in the first place. So, maybe this could be something more like:
Is the code logic being optimized something that should be prevented from being added to the app in the future, for example, through an ESLint rule?
The answer might be "no", which is OK, but it should at least be considered.
There was a problem hiding this comment.
A lot of times, it's just because it wasn't pre-optimized in the first place
That's def a good point 👍
How about we phrase it like this?
**Future 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)
Co-authored-by: Vit Horacek <[email protected]>
Co-authored-by: Tim Golen <[email protected]> Co-authored-by: Vit Horacek <[email protected]>
…ify/App into beaman-addPerformanceProposalPlan
…manceProposalPlan
| - [ ] > 20% reduction in Render Count | ||
| - [ ] > 20% reduction in Execution Time | ||
| - [ ] > 100ms reduction in Perceived Latency |
There was a problem hiding this comment.
IMO reducing render count/execution should result in gains in perceived latency. The user doesn't care if the Render count went from 300 to 1, they care if the app is 300ms faster, so let's track just that one metric. I think this would help eliminate a bunch of proposals for Reduced render from X to Y that doesn't really change App performance
There was a problem hiding this comment.
@luacmartins please feel free to bring up this point in the predesign discussion point here if you'd like to update this. We already got 7 👍 's for this point but we can always reconsider if you would like to provide your reasonings there
contributingGuides/PERFORMANCE.md
Outdated
| ___ | ||
| 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! |
There was a problem hiding this comment.
and email [email protected] to request access to our Slack channel!
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
If you would like to join our #expensify-open-source Slack channel, fill out this form with your email and Upwork profile link. If you haven't been added in 2 weeks, email [email protected].
There was a problem hiding this comment.
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!
| ## 3. Before/After Metrics | ||
| *Please fill out the table below. If a metric is not applicable, write N/A.* | ||
| | Metric | Before | After | Improvement | |
There was a problem hiding this comment.
The intention is to post this on slack, right? I think slack messages can't handle tables so it might be a pain to fill it AND it won't be readable to ppl
There was a problem hiding this comment.
Ooh yes correct this is meant for slack - def a good call... Seems there's another comment about this right below :D
| *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. | ||
| - [ ] **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)). |
There was a problem hiding this comment.
I think we could also ask contributors to provide more information about the environment of the test. What I mean is that if we get eg. 4 x CPU/network throttling or a slower device we'll get "better" numbers that may show higher improvement than with no throttling at all.
There was a problem hiding this comment.
Aah ok i like the idea of adding a new section explaining some details about the device(s) they used to test & prove their proposal provides a better performance somewhere. Do you have a suggestion exactly what device details they should provide?
There was a problem hiding this comment.
I think we could keep it simple: device name, CPU and RAM. We can also point out that all the measurements must be performed without any CPU throttling 😄
| | **Render Count** | | | | | ||
| | **Execution Time** | | | | | ||
| | **Perceived Latency** | | | | |
There was a problem hiding this comment.
Maybe instead of a table we could use some tool and upload a JSON in the comment? Just to make sure that contributors don't choose only "the best numbers".
There was a problem hiding this comment.
I think that's totally fair, what tool would you suggest?
There was a problem hiding this comment.
In general in this case I think the current version can stay as it is to not cause much delay, but I think it's worth to consider the options that we may have!
Internally we're using a tool that our colleague @sumo-slonik has built to compare the commit number and render times from the gathered profile traces. I think the contributors could simply add a screenshot from the tool and the profile traces that were used to generate it. It works great for the component performance, although with a single function execution time it might be a bit more tricky 😄
I usually rely on SpeedScope for analyzing web performance traces, but I’m curious whether others have a faster or more reliable way to measure function execution time—for example, across 100 runs—without significantly modifying the code? 😄 cc: @adhorodyski
Co-authored-by: Carlos Martins <[email protected]>
Explanation of Change
Creates template for contributors to propose performance improvements in our App product
Fixed Issues
$ #66161
Tests
None needed, this just adds documentation
Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari