Repository navigation
[HOLD for payment 2024-06-28] LOW: [Tracking] Metric - Send a message TTI (Performance audit by Callstack) #35225
Description
Intro
As a part of the Callstack performance audit, the issue tracks and summarises phases for Send a message metric.
The motivation behind measuring TTI of this particular behaviour were many reported issues of bad UX whenever user tried to send a message. By having an offline-first approach, we can also discard the network latency in measurements, but keep an eye on optimistic/actual updates in general during analysis.
Issue Owner
Current Issue Owner: @muttmuureMetadata
Metadata
Assignees
Type
Projects
- Status
Activity
kacper-mikolajczak commentedon Jan 26, 2024
Measurements
Details
Metric: Send a message TTI (ms)
Phase: Measurements
Commit hash: 76e5b1c
Workflow
Markers
Below is the list of defined markers that were used during testing:
messageProcessing: Time between Send the message(4) and Wait until message is displayed(5) steps of the workflowTheir implementation can be found in e8682e8.
The
messageProcessingmarker needed an extra metadata that were stored in thePERFmodule, which can later be used as a part of analytics. If that can be avoided, please let me know so we can improve/replace it 🔧 👍Measurements
Measurements were gathered by running the aforementioned
Workflowin10runs.The average time of
messageProcessingis: 14512.18 msAuxiliary data
Additionally for broader perspective, the resources consumption during one of the test runs is included.
mountiny commentedon Jan 26, 2024
@kacper-mikolajczak Thanks! Sorry if I am misunderstanding this but did it take 14 seconds from clicking Send button to show the message in the report in your testing? 😱
I have never experienced delay remotely close to this
adhorodyski commentedon Jan 26, 2024
It may be the Heavy account setting we're using
eduardo.graciano+newdotperf@callstack.com- I saw this myself where app was barely usable at this point (sending a message was hardly doable). The measurements provided by @kacper-mikolajczak unfortunately match my own experience in a chat on this account.This was all on a physical device, production build and this exact account. @rinej will post a recording soon so we can see how the experience looked like.
kacper-mikolajczak commentedon Jan 26, 2024
Hi @mountiny 👋 No misunderstanding whatsoever - I did not believe at first as well :D
As @adhorodyski already pointed out, measurements were aligned with the guidelines but the app with this specific setup is not performing as it should. I will post the analysis soon, just to unwrap of what is going on under the hood 👍
rinej commentedon Jan 26, 2024
@mountiny I am sending video example with around 15s for sending message.
It is done on real device and on
eduardo.graciano+newdotperf@callstack.comshort-flow.mp4
Additionally on that account sometimes sending message is bugy. It sends just part of the message and keeps the whole message in the input
Example videos:
bug1.mp4
bug2.mp4
mountiny commentedon Jan 26, 2024
Holy Moly
adhorodyski commentedon Jan 26, 2024
Give us some time we're already cookin' something!! 😊
kacper-mikolajczak commentedon Feb 2, 2024
Analysis
In order to analyse underlying issues on Android platform, the app was ran in release build variant and profiled using both native and JS tools (Android Studio Profiler, Chrome Profiler and React Profiler respectively).
As a brief overview, here is a high-level representation of analysis flow:
Finding the dark side
Android Studio Profiler is used to determine if performance issues stem from native, JS or both sides of execution.
The results of native profiling when plugged into Perfetto visualiser look as follows:
The focus of the analysis is bound to the 4 main threads from React Native standpoint:
mqt_js(JS Thread)UI Threadmqt_native_modulesRender ThreadBy looking at the CPU usage summary, there are two areas of high-density calculations. The biggest concern seems to be the overload of
mqt_jsthread in the two aforementioned timeframes, which analysis will look into the most.JS Profiling
The detailed JS profiling trace is obtained thanks to
react-native-release-profilerlibrary, which enables to profile the Hermes (JS) thread in release build. The trace will be presented viaChrome Devtools, which in turn, is going to be helpful determining the exact root cause(s) of an javascript-land issues.From the trace observed above, the key take-away were made:
The main goal of the analysis is the reduce the amount of calculations as much as possible.
Each section down below contains information about specific finding and its results on overall performance. The results will be summed up between sections to eventually present the final outcome in very last part.
After each section with meaningful changes in timings, the table will be presented like one below. First (baseline) and the last (final) tables consist of 10 samples, while the intermediate ones have 5 samples for brevity.
Hermes Intl
localeCompareHighest amount of work is performed by native string method
localeComparewhich, summarized from all calls, takes about 23 seconds to complete. The issue stems from Hermes engine Intl issues which are described more in detail in this proposal by Hur who found the original fix.Result of fixing the issue is drastic reduction of time required to complete all the calculations. We were able to reduce workload from 27 seconds to about 5 seconds.
The 3 bumps
While previous step carried out enormous gains, we still have quite a lot of work going on under the hood. Even with such reduction we still can observe the pattern when looking at the JS profiling trace - there are three significant overheads when sending the message. After investigating, those “bumps” seem to be:
hasDraftproperty in report object when starting typing a messageEven though those bumps are a result of the different application flows, they have the same root which is tied to the
SidebarLinksDatacomponent and its internal calculations.The
SidebarLinksDatasubscribes to collection of allchatReportsfrom Onyx (in testing account about 14k of reports). Collection is invalidated every time one of the report changes, which happens in aforementioned bumps (updating thehasDraftproperty and sending the actual message flow).Whenever
chatReportscollection is invalidated, it breaks up the memoization of heavyoptionListItemsobject. Keep in mind that the memoization might also break because of other dependencies. It was not the case during the analysis of sending the message, but they appear as fragile aschatReports.SidebarLinksDataordering mechanism and its pitfallsNew Order
The major suspect of
optionListItemsbad performance seems to be thegetOrderedReportIDsmethod fromSidebarUtils. It is called to calculate new order of reports forSidebarLinksDatawhenever dependencies change.Looking at the stack trace, the
getOrderedReportIDsfunction call is responsible for biggest time overhead of about 2.5-3s per call.getOrderedReportIDsoverhead inside SidebarLinksDataEach call consists of number of operations performed on set of reports (and other arguments) to determine correct order of reports:
JSON.stringify)arrayPrototypeFilter)arrayPrototypeForEach)arrayPrototypeSort)Those calculations are done for all of thousands reports every time collection is invalidated, thus resulting in high overhead. Let’s look closely at those in sections below.
The Cache
As old joke says, the three hardest things in computer science are cache invalidation, naming things and off-by-one errors. The audit, based on previous findings, will try to address them in this section.
The
getOrderedReportIDsfunction tries to avoid calculations by introducing a cache. While it is generally a good practice, in this specific example it might add more overhead that it tries to mitigate.App spends about 150ms performing
JSON.stringifyon heavy list of data in order to compute thecachedReportsKey.Then the generated key is checked against existing cache. If cache entry is hit, the cached ordered
reportIDsare returned. The issue here is that the cache entry is unlikely to be hit, especially given maximum cache size of 5 elements and quite big amount of input variables:currentReportIdallReportsbetaspoliciespriorityModeallReportActionsBy removing the caching mechanism, the time required to calculate
getOrderedReportIDsgets decreased by about 0.3s (~15%).Additionally, the
allReportActionsare solely used to compute the cache, so when removing the cache implementation, the entireallReportActionsretrieval process can be omitted as well (including request to Onyx), which is another potential gain for entireSidebarLinksData. By comparing the stack traces there is ~40ms gain in performance:allReportActionretrievalallReportActionretrievalThe naming
Another heavy lifter inside
getOrderedReportIDsis a computation of report’sdisplayNameproperty viagetReportName.forEachthat is mostly related togetReportNamecalculationsIn order to calculate the display name,
getReportNamecalls internally many different helpers, but the ones that stand out the most aregetDisplayNameForParticipantandgetPolicyExpenseChatName.getDisplayNameForParticipantandgetPolicyExpenseChatNameoverhead of ~200msWhile one could try to squeeze out gains by looking for a more performant library or doing micro-optimisations, the best way to save time is to calculate static properties once (on server-side preferably), as they are not intended to change and derive dynamic values from them.
For example, in
getDisplayNameForParticipantimplementation we can see that the value returned fromformatPhoneNumbercall can be re-used:The
formattedLoginfield could be part of the personalDetails itself or might live in a separate entity. As an experiment, the formatted login was stored in a makeshift map and it resulted in about 500ms of reduced overhead:The example brought up here are meant to illustrate what can be done on broader scale in the implementation phase with app’s data flow in general. The higher in the tree of data sources we can reduce the number of calculation the better and this is something we will do in future.
Report’s
hasDraftpropertyAn example of such high-level improvement could be process of determining whether report has a draft message. It is fired whenever user starts typing a new message and is responsible for the first of three bumps described above.
Updating draft state is mainly used to show correct order and UI in
LHN. While it is an important part of the UX, we don’t necessarily want to perform 1.5s of calculations to update one property.Unfortunately, the
hasDraftproperty is strictly bound to the report object. Changing the (part of) report will result in expensive calculations of aforementionedgetOrderedReportIDs.In order to fix this, the
hasDraftproperty could be extracted away from the core report structure and be served as a separate dependency.Here are the results of mitigating the
hasDraftimpact - the first bump of calculation that was present in one of the first measurments is now almost completely gone. The reduction goes from about 1.5s to under 100ms of an overhead. In comparison of other bumps, it now looks negligible.![Results of avoiding the unnecessary
getOrderedReportsIDsupdates on draft message]getOrderedReportsIDsupdates on draft messageThis was achieved thanks to replacing the
hasDraftproperty with a separate Onyx keyREPORT_HAS_DRAFT_which is then used in the component of interest (e.g.SidebarLinksData,OptionRowLHN). The most significant improvement is made by skipping the costly call ofgetOrderedReportIDswhenever the draft status changes - instead of calculating entire list from scratch, the newly created draft is injected into previous list.Here is the partial implementation seen from
SidebarLinksDatacomponent:SidebarLinksDataimprovementsSimilar pattern was applied to other draft properties, like
REPORT_DRAFT_COMMENT, so it might be a worthwhile to spark a discussion about handling such structures in the system.Summary
While there is still a room for improvements, as the goal the we are striving for is 100ms of execution time, with the proposed changes involved, sending a message time went down from ~15s to ~2.5s.
Such changes are meant to be taken as a general direction or example of implementations which will be continued in next phase of the audit.
The main goal is to reduce costly calculations that freeze the thread making app unresponsive for the user - which is even worse UX than inaccessible inputs! Following that, as presented with smaller improvements we aim to improve both performance and development experience through writing more reliable code.
Hope you like the analysis - see you soon in the implementation phase! :)
CC @mountiny @roryabraham
mountiny commentedon Feb 2, 2024
These analysis are really impressive and great opportunity for everyone to learn from too. Its also great to see that this is in the open source channel as new engineers/ anyone can get inspired by these.
I am very excited for bringing the proposals from this into the App!
kacper-mikolajczak commentedon Feb 6, 2024
Love to hear that @mountiny!
We are also eager to implement the proposal as soon as possible to get the benefits out of it. Before that, would you like to break down analysis with me into some Actionable Points we can treat as entries into Implementation Phase?
For convenience, here is a quick summary of main points we can try to make into AP and prioritize:
Intlimprovements (order of magnitude improvement,~12s)getOrderedReportsIDstweaks: (~1.2s)displayNameformatting overhead~200ms-~800ms)Reportobject into separateMetadatastructurePlease keep in mind the actual time gains might roughly differ - their purpose here is to have a general overview of at what level presented change might impact the performance 👍
mountiny commentedon Feb 6, 2024
Thank you Kacper, I think my reasoning for ordering would be: make the biggest impact changes first and also prioritize the lowest hanging fruit. Seems like your list achieves that so let's do that
kacper-mikolajczak commentedon Feb 6, 2024
Great, I find such ordering best as well. Just to sum up, here are ordered Action Items:
IntlimprovementsgetOrderedReportsIDstweaks2.1. cache removal
2.2.
displayNameformatting overhead2.3. sorting improvements
Draft messageoptimisations3.1. moving draft-related properties from
Reportobject into separateMetadatastructure3.2. Improving dependency management (implementation of
useMemo)We are going to use those items as an input into Implementation Phase
CC: @mountiny
mountiny commentedon Feb 6, 2024
Thanks!
14 remaining items
muttmuure commentedon Jun 12, 2024
@kacper-mikolajczak are you still on this one?
kacper-mikolajczak commentedon Jun 12, 2024
Hi @muttmuure it was part of the last performance audit and is completed for now.
muttmuure commentedon Jun 12, 2024
Closing
[-]LOW: [Tracking] Metric - Send a message TTI (Performance audit by Callstack)[/-][+][HOLD for payment 2024-06-28] LOW: [Tracking] Metric - Send a message TTI (Performance audit by Callstack)[/+]melvin-bot commentedon Jun 21, 2024
Reviewinglabel has been removed, please complete the "BugZero Checklist".melvin-bot commentedon Jun 21, 2024
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.85-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-06-28. 🎊
For reference, here are some details about the assignees on this issue: