Performance issues with the new label screen #819
Comments
|
|
I suspect that the reason behind this is that we are loading the data in the directive, which slows down the data load overall. |
|
Here's what I see in the logs around 12:30. Lots of calls to Bunch of calls to read the location for the delayed walk check Filtering all those calls out, we get Each individual call does not appear to be too slow, but the combined calls take two minutes to complete. We then call configReady multiple times which is presumably when we switched back to "To Label". By putting the data load into the directive, we ensure that we read the data again Some of the other logs related to this are with |
This doesn't have timestamps so it doesn't tell us when the directives finish initializing. |
|
Tried to reproduce in the emulator with all logs, and it is not 100% clear that the directive creation is the problem. Let's try changing that and see how it works. Here's where the list controller is initialized Read the range Read all the inputs Matching against unprocessed trips List has loaded Started creating trip items Started reading location data in parallel Started and finished creating directives Some location reads were launched after that |
|
Changing to
|
|
And arguably, moving the code to display the directive into the directive is the right thing to do structurally. So basically, the list should be in charge of reading the trips and the directive should fill in the trip information. Let's move it out and see if it helps before deciding how to proceed. |
|
Ok, so I now know why "Address is visible but not other fields" It is because we wait until we get the trip back and then we populate the value that is actually referred to in the template Simply assigning that earlier or just using |
|
Looking at the logs, moving the code to read the entries outside the directive does not seem to load the trajectories sooner. Let's first try with leaving the code that reads the data in the directive, but change everything to just use the |
|
Code to move to the list (if we need it again) is attached here: |
|
Waiting to see if this actually works |
|
Another issue that I just thought of is that putting the retrieval into the directive means that the data will be reloaded every time the directives are recreated. This is acceptable if we are recreating the values after a full reload, but as we switch between "To Label" and "All Trips", we have the same trips, but the We can add a fairly simple fix, in which we check to see if the trajectory has been retrieved before we try to re-retrieve it, but that will not solve the problem with outstanding requests in the scheduler. This is almost certainly the reason why we get the hang in "All Trips" and the delayed "Reading from server" calls. There were pending requests from the "To Label" that needed to be resolved before we started loading in all trips. I checked the bottleneck library and there does not appear to be a way to cancel in-flight requests. So maybe the best option is to retrieve in the list view after all. |
|
Running this in the emulator, we get After restarting, we get where the line is |
|
This seems to happen between |
|
It looks like this is from the HTML/ |
|
|
|
Through experimenting with removing code, I can confirm that works and does not commenting that out (so that we will be able to push this out). |
|
Pushed out the changes, still did not fix it. On android phones, "To label" works pretty well On the staging iPhone (iPhone 7), starting the app hangs on "To Label" I checked the server logs, and here's what I see (modified slightly to match the START and END timestamps, with lines indicating the batches. It looks like actual calls were only milliseconds long, so that should not be the reason why anything hung. I wonder if it is the bottleneck library that is causing the problems... |
|
Tried on my personal android phone. We made 23 calls There are 20 trips (all labeled), so, including the calls to read mode and purpose, that seems pretty decent. Next, let's load some more trips in "To Label": "All labels" now shows all trajectories but does not load map tiles. Load some more data: UI hung for a while, then went to the detail screen. Loaded even more trips; now displaying 58 trips. Only one more call to the server; to read the trips. No more trajectory calls, all maps are grayed out. Ah wait! 5 mins later, we get a couple of other calls |
|
And then a couple of minutes later, we see a bunch of others Trajectories filled in now, but we got an SSL error while connecting to the server as well. |
|
Pressed one more time, And again, there were no trajectory loads for close to 15 mins - when I launched the app 15 mins later, suddenly all of them were dispatched. This definitely seems to indicate some issue with the bottleneck library. Pressing the button again at :45 |
|
Reading the confirmed trip Kept the app in the foreground and ~ 5 mins later, the trajectory calls started Pressed the button again at x:52 This time the load was fairly fast Clicking on the detail screen seems to unblock the bottleneck sooner? |
|
Current situation:
Hacking this by truncating only the most recent few entries ensures that all the entries are loaded pretty quickly and that we can scroll. However, this is bad because then we will essentially load some trips twice. And what will happen if/when the user actually clicks on the button on the top to pull more data, or goes to "All Trips". We then need to keep track of where we are in the scroll, and remove or add directives as we scroll. But isn't that what |
|
ng-repeat is known to be a big performance issue on angular, and the workaround is to use |
|
Using |
|
Fixing the one location where we need to change that, we can now display the items correctly and we correctly scroll to the bottom, BUT scrolling is still a bit janky and we cannot scroll until all the trajectories are loaded. Removing the |
|
Commented out |
|
Nope, changed from
but if I comment out the call completely, then there are no hangs. Is this some weird directive thing where the directives are not loaded until all their data is in place? If so, what data are they looking for? |
|
Ok, so this hang is either due to:
Let's try removing each of them and see what happens. |
|
Removed bottleneck, and "reading data" is a bit slow since we launch all the tasks before we are done. But all the results come back super early, so it is not clear if we are hung Let's add some timeouts to slow this down. |
|
Ok, with adding timeouts, the UI hangs until all the calls are scheduled. Even the popovers are not scheduled. This seems to indicate that the problem with the scheduler, which apparently takes a while to return...? |
|
Replacing the timeout with the mapLimiter, we get: I also tried this again, and tried to type out javascript on the command line (e.g. Why did this not happen for the place reads? |
|
Confirmed that this does not happen for the place reads. They are scheduled after the list is displayed. why are all trajectory reads launched before and the place ones are not? I bet this is because of the way we handle the callback in the two cases, which had been my suspicion all along. |
|
Changing that does change the invocation order, and we see the trip with the mode and purpose fairly soon. BUT the UI is still hung until the calls are complete. At this point, it seems to be a contention issue between the reads and the UI display. Screen.Recording.2022-10-25.at.10.25.04.AM.mov |
|
Yup! Javascript is single threaded, so the callbacks from multiple threads running in parallel slow down the user interface. The advantage with (1) is that it will help us with performance issues in general (1) seems like a more principled solution overall, especially if we want to pre-read and cache values while the user is labeling the current set. Let's try it. We may try (2) anyway when we change the trips to show the sections as well. Note also that (1) is likely to address current performance issues with jankiness due to reading from nominatim as well. |
|
Note that part of this might also just be that the values return super fast when we are connected to localhost. However, it seems like it would be good to not cause phone slowness when the server is actually fast. What's the point of optimizing the server then?! |
|
I can successfully launch a worker in a non-angular module. I cannot launch it in an angular module. Let's stick with the separate file for now and figure out how to integrate with angular1 later. |
So that we can reuse existing directives after re-initializing, which improves performance. https://www.codelord.net/2017/01/25/techniques-for-improving-ng-repeat-performance/ aquint/ion-alpha-scroll#5 https://medium.com/littleiffel/angularjs-my-solution-to-the-ng-repeat-performance-problem-7a3534658bbe This means that the directives are initialized without a trip, so we need to check for trip existence before accessing any trip fields e-mission/e-mission-docs#819 (comment) e-mission/e-mission-docs#819 (comment) e-mission/e-mission-docs#819 (comment)
|
ok, so it looks like the And I get the error Given that, it is not clear how we can access the cordova plugins. Let's try to pass in the plugins |
|
|
|
ah we cannot call cordova APIs from a webworker and this is currently marked as We could switch to standard XMLHTTPRequest calls, but then we will run into CORS issues with cordova WKWebView. I am not sure we want to go backwards on that, and I am also not sure that we can support CORS easily on the NREL environment. Given this, I think we should probably go to option (2) from #819 (comment) We can use web workers for the address reads and (2) for the trajectory reads. |
|
Since we are working on web workers right now, let's finish the address reads. We could even temporarily address |
|
Actually, I am not sure that it helps to use web workers for the address reads either. Note that the actual call to the server is already async. So presumably the slowness, if any, is coming from processing the entries on the phone, and that happens automatically in Let's first see how bad the current situation is, and then decide how to proceed. |
|
the current situation is that the previous issue with the delayed reads is addressed The queries seem to come it at a consistent cadence. However, at least on the emulator, the UI is not responsive until all the trajectory information is handled. If this is indeed due to the Let's:
|
|
Installed the current code on an actual phone; seems to work just fine. Let's try pushing it out and see if we are done. |
|
On the emulator, the devapp had been getting slower and slower, to the point that it couldn't even start up. So I uninstalled and reinstalled, and everything is super-fast now even in the emulator. All 40 trips are loaded without any problems with performance or hanging. |
|
Merged and pushed to production |
Performance issues with the new label screen
The text was updated successfully, but these errors were encountered: