Skip to content
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

Nearby activity feels slow, performance issues #6038

Open
nicolas-raoul opened this issue Dec 16, 2024 · 8 comments
Open

Nearby activity feels slow, performance issues #6038

nicolas-raoul opened this issue Dec 16, 2024 · 8 comments

Comments

@nicolas-raoul
Copy link
Member

nicolas-raoul commented Dec 16, 2024

  • UI slowdowns in areas with many points
  • slow loading of pins even cached ones
  • memory leaks

Some issues have been identified, feel free to report others:

@savsch
Copy link
Contributor

savsch commented Dec 17, 2024

Throughout this comment, I will use the terms "pins: initial loading" and "pins: details loading" to refer to the two stages in which pins are loaded in nearby.

Spent the greater part of my day trying to fix UI slowdowns (caused mostly by flawed "pins: details loading" implementation).
This is the branch I have been working on. (didn't create a draft PR as it started out as an experiment and rewriting some of the marker overlay management is still left.)

Rewrote most of the "pins: details loading" logic, the existing implementation of which was computationally inefficient in part and also prone to race conditions.
This should hopefully have fixed most UI slowdowns.

@nicolas-raoul can you try out the branch and confirm whether the UI slowdowns have lessened?


Stuff that's still left to account for:

  • Map tiles loading slowly.
  • Issue tracking the current location.
  • Flawed "pins: initial loading".

I'd like to expound upon the last point above, and also suggest fixes. Creating a new issue for the same... EDIT: Done #6044

@nicolas-raoul
Copy link
Member Author

@savsch I just tested your repo, it seems so much faster! 🎉

@nicolas-raoul
Copy link
Member Author

nicolas-raoul commented Dec 19, 2024

With your repo:

  • Initial loading:Zooming out then in does not trigger slowdowns anymore. 🎉
  • Details loading: Cached pins load very fast. 🎉

@nicolas-raoul
Copy link
Member Author

nicolas-raoul commented Dec 19, 2024

I spotted another improvement opportunity.
After initial loading, the current algorithm for loading details of pins seems to be:

  1. Start from the centermost pin.
  2. If it is cached, then show it, else make the query and show it.
  3. Move on to the next pin, going away from the center little by little.

Instead, I would suggest this algorithm:

  1. For each pin, check whether it is cached or not. Show all cached pins.
  2. Now we only have uncached pins left. Similarly to the algorithm above, perform queries starting from the center.

Current behavior, observe how the big group of pins near Kilwa becomes grey and stays gray for 40 seconds despite being available in cache:

screen-20241219-164239.mp4

@nicolas-raoul
Copy link
Member Author

Still on your repo, I sometimes see the loading icon forever:

screen-20241219-103626.mp4

@nicolas-raoul
Copy link
Member Author

Still on your repo, after leaving Nearby open for 10 minutes, it suddenly crashed:

APP_VERSION_NAME=5.0.2-debug-nearby/performance-experiments
ANDROID_VERSION=15
PHONE_MODEL=Pixel 9 Pro
STACK_TRACE=java.lang.RuntimeException: java.lang.OutOfMemoryError: Failed to allocate a 24 byte allocation with 1516256 free bytes and 1480KB until OOM, target footprint 536870912, growth limit 536870912; giving up on allocation because <1% of heap free after GC.
at fr.free.nrw.commons.concurrency.BackgroundPoolExceptionHandler.lambda$onException$0(BackgroundPoolExceptionHandler.java:18)
at fr.free.nrw.commons.concurrency.BackgroundPoolExceptionHandler$$ExternalSyntheticLambda1.run(D8$$SyntheticClass:0)
at java.lang.Thread.run(Thread.java:1012)
Caused by: java.lang.OutOfMemoryError: Failed to allocate a 24 byte allocation with 1516256 free bytes and 1480KB until OOM, target footprint 536870912, growth limit 536870912; giving up on allocation because <1% of heap free after GC.
at fr.free.nrw.commons.logging.FileLoggingTree.logMessage(FileLoggingTree.kt:66)
at fr.free.nrw.commons.logging.FileLoggingTree.log$lambda$0(FileLoggingTree.kt:55)
at fr.free.nrw.commons.logging.FileLoggingTree.$r8$lambda$XsoUiE8XuqISALIIa-wwDdY9dms(Unknown Source:0)
at fr.free.nrw.commons.logging.FileLoggingTree$$ExternalSyntheticLambda1.run(D8$$SyntheticClass:0)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:487)
at java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:307)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
... 1 more

@savsch
Copy link
Contributor

savsch commented Dec 19, 2024

Thanks for the feedback.

Instead, I would suggest this algorithm:

1. For each pin, check whether it is cached or not. Show all cached pins.

2. Now we only have uncached pins left. Similarly to the algorithm above, perform queries starting from the cente

Yup, had this at the back of my mind as well.
I have been making changes to the branch, and have implemented this approach.

Also, made the pin-loading parallel, so 3 batches (each sized 3 itself) will be processed at once (i.e. 3 parallel connections). That seems to work better for me.

Still on your repo, after leaving Nearby open for 10 minutes, it suddenly crashed

Couldn't reproduce it on my current HEAD. Maybe it's because I've removed the hacky JustExperimenting class and moved that functionality over to the presenter. Hopefully the OOM is gone now.


The logical work is done I suppose, just adding javadoc is left, and some tests won't compile yet because of the changes. Going to make a draft PR.

@savsch
Copy link
Contributor

savsch commented Dec 19, 2024

Still on your repo, I sometimes see the loading icon forever

With the current HEAD, it should not happen any more frequently than it does on the current main. Even if it does happen, I am likely to replace the existing loading icon implementation, as it's a bit hacky in my opinion (relies on Handler.postDelayed and static variables), when working on #6044.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants