feat: performance improvements #787
Conversation
| @@ -163,12 +164,17 @@ export const EditForm = ({ | |||
| ); | |||
| }; | |||
|
|
|||
| export default function Form({editForm = false, onInfoPress}: Props) { | |||
| // TODO(performance): Adjust types | |||
There was a problem hiding this comment.
What sort of adjustments are needed?
There was a problem hiding this comment.
Nothing major, just regular props type adjustments I omitted here to save a bit of time
There was a problem hiding this comment.
thanks igor! the suggested changes all make sense, i left a couple questions about the details of implementing some of them if you have time to respond. if you don't have time to keep working on the PR i'll take it over
were you able to benchmark the app and identify any performance bottlenecks any particular performance bottlenecks? and did the changes you made here make a significant difference in iOS performance or is there still more work to do to get things usable on that platform?
| {/* TODO(performance): | ||
| 1. All text inputs should be adjusted into uncontrolled components | ||
| - https://react.dev/learn/sharing-state-between-components#controlled-and-uncontrolled-components | ||
| - https://github.com/facebook/react-native/issues/20119 | ||
| 2. The code should be rewritten/reorganized/memoized so that a change to a text input's value doesn't | ||
| trigger a re-render of the whole Form/Component (chokes the JS thread and results in terrible frame drops) | ||
| */} |
There was a problem hiding this comment.
could you expand on how this would work? the form components aren't controlled by props when they're in a form, and are memoized. or are you saying that the entire architecture of Formik is causing the forms to re-render on each keystroke and we need to move away from the library?
There was a problem hiding this comment.
Sure, I'll try to elaborate on this further next week
| // TODO(performance): Consider adjusting ProjectPreviewCard with the design team | ||
| // to always have a fixed height so you can leverage getItemLayout (example below): | ||
| // const getItemLayout = useCallback((_: any, index: number) => { | ||
| // const length = ITEM_HEIGHT + SEPARATOR_HEIGHT; | ||
| // const offset = length * index; | ||
| // return {length, offset, index}; | ||
| // }, []); |
There was a problem hiding this comment.
this is kind of a general UI question, but if you're specifying fixed heights in pixels in advance, how do you handle something like a user who has set a large font size preference for accessibility reasons?
There was a problem hiding this comment.
Need to look into that, but off the top of my head, I think it doesn't have to be fixed-fixed but rather deterministic and repeated for all items, great question though
There was a problem hiding this comment.
i see, so it'd work but you'd need to find a way to compute that dynamically. i've run into this problem with other UI frameworks before, never understood why there can't be an option to say "i promise they'll all be the same dimension" instead of "i promise they'll all be this dimension" so the framework can just measure one item's layout and use that value for the rest of the list which seems much less error-prone
shrouxm
force-pushed
the
feat/performance-improvements
branch
from
8c74e64
to
e67891a
Compare
There was a problem hiding this comment.
thank you @igorbej! i've started working on my own PR #823 based on this one with further work, including replacing some of our usages of NativeBase components. i rebased this PR onto current main, but just saw you said you might do some more work this week so hopefully that won't cause any conflicts! (the rebase was conflict free so i think it'll be ok). you can check out my PR to make sure we aren't doing any overlappping work, besides what's pushed there i'm now just working on replacing some more NativeBase components because that seemed to be the most impactful change in terms of overall performance improvements (as opposed to fixing bugs with specific interactions). if you don't have time, i'm happy to take over this PR and merge it this week!
|
@igorbej one thing i was curious your opinion on, i enabled react navigation's react-freeze support in my PR. it made a huge difference with a few problem screens, but it's marked experimental, do you have any thoughts on pros/cons of the feature for our app? |
|
@shrouxm heads-up that I'll be looking into this one and your comments again today/tomorrow, but right off the bat, I just wanted to let you know that (as a general rule) you should feel free to adjust/reuse/merge anything that I contribute as you see fit. As long as I have some kind of update to know what you're working on, I'll be fine — if I have anything to commit, I can always create another PR, and I definitely don't want to block you folks 😄 |
|
@igorbej ok sounds good! some updates from today:
|
|
@igorbej ok team confirmed this is resolved on all (our) devices so we're switching focus back to feature work & no need to keep working on it! i'm just going to spend today getting these PRs merged and fixing any style regressions thank you so much for your insight on all of this! |
|
@shrouxm Great to hear that! I didn't manage to carve out enough time to come up with a meaningful update yet, but I'm still dabbling (checked out your branch, looking into those Formik forms re-renders atm), and I'll continue to for as long as I have some availability on my hands, as I appreciate the effort and the project 😁 I'll let you know when I have something to share, and if anything comes up in the meantime, feel free to hit me up here or on Slack. Cheers 😄 |
shrouxm
force-pushed
the
feat/performance-improvements
branch
from
e67891a
to
b606b79
Compare
|
closing as these changes were merged into main with #823 |
This PR relates to the performance issues discussed in #120
Summary
Details
To reiterate and to be extra explicit: I haven’t been able to replicate an identical iOS lag/freeze as the one in #120, but judging by how that looks in the recording, but also the app’s behaviour, its patterns of using system resources, the load it puts on the main and the JS threads, my current theory is that it’s not a singular issue causing the lags, but rather multiple unrelated issues compounding to slowly but surely smother the app (especially the JS thread). And that results in lags and unresponsiveness like the one you saw in your tests, where the app’s business logic part (i.e. JavaScript) is so overwhelmed it doesn’t appropriately process the touch events (i.e. button presses) coming to it from the native world.
The issues
Of the issues I found: some I resolved immediately; some, specifically ones that required more time than I had available or some discussions/decisions on your end, I solved partially and/or marked in code with comments.
Here's what I've noticed:
valueprop) which is mostly fine on the web, but can be problematic for React Native due to its asynchronous nature and the fact the JS <> Native communication is happening over the bridge (see here and here for more context). And since the app is struggling already, it is indeed problematic here, resulting in input fields being extremely laggy, sometimes to the point of updating only after a multiple-second delayiOS vs Android
I would argue that Android suffers from the same performance problems as iOS as they are mostly JS-specific, and during my tests I could, for example, easily crash the Android emulator (debug build, Google Pixel 3) due to insufficient RAM after creating just a handful (~4-5) of new sites. Frame drops and some lags were also present.
That being said, the particular interaction of switching back and forth between the HomeScreen and ProjectListScreen tabs, which in your implementation uses the Stack navigator (rather than the Bottom Tabs navigator appropriate for this use case, which we've talked about in #698), is different on Android and iOS (a sort of fade-in on Android vs a slide from the edge of the screen on iOS), as the underlying native primitives also differ. And these differences might be the reason why the overall poor performance of the app affects this interaction unevenly on the two platforms. Although I have to say I'm speculating here, this isn't a conventional use case and I'd encourage you again to consider using Bottom Tabs for your bottom navigation (or Material Bottom Tabs if animations/transition effects are what you're looking for).
Results
iOS
main(debug build, but still)(WIP)
Android
As noted earlier, I had no trouble getting Android to 700MB+ memory usage too, at which point it was crashing:
Providing it with more memory resulted in bigger consumption:
Plus the overall experience was laggy, just like the iOS one, therefore I'm sceptical all of these issues are relevant for iOS exclusively.
Other remarks
Checklist
Related Issues
Relates to #120
Verification steps
(^Added by @shrouxm, I sincerely hope the app can clear this bar with these changes, but it is possible that more work will be required to achieve that for every scenario/interaction.)