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

Unusable performance with >1k items in ClusterManager #1199

Open
mbilalhussain96 opened this issue Jun 30, 2023 · 16 comments
Open

Unusable performance with >1k items in ClusterManager #1199

mbilalhussain96 opened this issue Jun 30, 2023 · 16 comments
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@mbilalhussain96
Copy link

mbilalhussain96 commented Jun 30, 2023

If I add over 1k items to the ClusterManager, the actual (drawn) markers are updated after very long delays. For example, I can have one cluster with "1000+" text on it, and when I zoom in, it stays for about 1-2 minutes, and only after that it splits into smaller clusters and markers.

  private lateinit var mMap: GoogleMap, 
   private lateinit var clusterManager: ClusterManager<mapclass>

  private fun addItems() {

        // welcomedataItemList1=1000

        for (place in 0 until welcomedataItemList1.size) {

            if (welcomedataItemList1[place]?.lat != null && welcomedataItemList1[place]?.lon != null) {

                var latitude =welcomedataItemList1[place].lat.toDouble()
                var longitude =welcomedataItemList1[place].lon.toDouble()
                serial = welcomedataItemList1[place].serial
                ownerName = welcomedataItemList1[place].ownerName
              
                status= welcomedataItemList1[place].adminStatus.toInt()
                mMap.moveCamera(CameraUpdateFactory.newLatLngZoom(LatLng(latitude, longitude), 6f))
                var offsetItem = mapclass(latitude, longitude, "$serial", "$ownerName")
                clusterManager.addItem(offsetItem)

            }
        }

    }


private fun setUpClusterer() {

        val metrics = DisplayMetrics()
        windowManager.defaultDisplay.getMetrics(metrics)

        clusterManager = ClusterManager(this, mMap)
        clusterManager.setAlgorithm(NonHierarchicalViewBasedAlgorithm(metrics.widthPixels, metrics.heightPixels))
        mMap.setOnCameraIdleListener(clusterManager)



        addItems()

        clusterManager.renderer = MarkerClusterRenderer(this, mMap, clusterManager)

        mMap.setInfoWindowAdapter(CustomInfoWindowForGoogleMap(this))

        mMap.setOnMarkerClickListener(clusterManager)
        mMap.uiSettings.isZoomControlsEnabled= true
        clusterManager.setAnimation(false);
        clusterManager.cluster()

    }


override fun onMapReady(googleMap: GoogleMap) {

      mMap = googleMap
            setUpClusterer()
        
        }
@mbilalhussain96 mbilalhussain96 added triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Jun 30, 2023
@wangela
Copy link
Member

wangela commented Jun 30, 2023

If you would like to upvote the priority of this issue, please comment below or react with 👍 so we can see what is popular when we triage.

@mbilalhussain96 Thank you for opening this issue. 🙏
Please check out these other resources that might help you get to a resolution in the meantime:

This is an automated message, feel free to ignore.

@wangela
Copy link
Member

wangela commented Oct 31, 2023

A suggestion from @cwsiteplan in the Compose issue discussion:

would it be possible to expand clusters only for the visible viewport? - seems like it's currently expanding on zoom level - adding lot's of individual markers on areas that are potentially not viewed at all

@wangela wangela added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed triage me I really want to be triaged. labels Oct 31, 2023
@wangela wangela added needs more info This issue needs more information from the customer to proceed. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Nov 17, 2023
@ColtonIdle
Copy link

is this fixed with googlemaps/android-maps-compose#421 ?

@kikoso
Copy link
Collaborator

kikoso commented Nov 27, 2023

@ColtonIdle , it is partially solved. Compose has an intrinsic lack of performance due to some limitations in the framework (i,e,, it has a penalty by using certain classes, and one of them is LatLng, which we use massively in a Cluster). So there is always going to be a level of lack of performance that we can't reduce (we are actually exploring this using a different mechanism, but that is another story).

This PR allows to choose another algorithm for the rendering. Namely, NonHierarchicalViewBasedAlgorithm only renders contain in the screen, which improves performance by a factor.

I have added a sample to the repository, for more clarity:

googlemaps/android-maps-compose#463

Feel free to check it out.

@ColtonIdle
Copy link

Hm. so if we care about performance. we shouldn't use compose for clustering?

@kikoso
Copy link
Collaborator

kikoso commented Nov 27, 2023

At the moment, there is a certain level of intrinsic lack of performance in Compose. Again, we are exploring this (specifically, a new feature from the Compose 1.5.4 compiler called strong skipping).

Unless you are trying to cluster thousands of them, Compose will likely work (as long as you are also using the NonHierarchicalViewBasedAlgorithm)

@ColtonIdle
Copy link

Yeah, I currently have an app where we have about 16k items (mostly US and europe) hence my curiosity because we're not (yet) using compose maps. but we are using a fully compose android app with the exception of the map =)

im really eager to ditch AndroidView though as I've had a bunch of issues with clusters not recomposing properly. thanks for the update. and strong skipping seems cool. i wonder if we can just mark latLng as @stable though?

@kikoso
Copy link
Collaborator

kikoso commented Nov 27, 2023

I would suggest you to check the Clustering with NonHierarchicalViewBasedAlgorithm, check out the PR I linked above. You can manually add another few thousands items, and see how the app behaves.

@ShikaSD
Copy link

ShikaSD commented Nov 28, 2023

Hi, one of the Compose devs here :)

Could you share a bit more about performance problems you have encountered? I am not personally familiar with Compose integration for Maps, but I'd be happy to chat about it and help debug / address perf issues with it.

@kikoso
Copy link
Collaborator

kikoso commented Nov 28, 2023

Hi @ShikaSD ,

The issue is happening in android-maps-compose.

It is likely related to this issue here, with the LatLng type not being inferred as stable.

Setting the stability via a config file and using strong skipping does not seem to fully remove the issue.

@ShikaSD
Copy link

ShikaSD commented Nov 28, 2023

@kikoso stability by itself does not result in performance problems that often, I doubt this is the cause here. I suspect the problem is in heavy content used for clusters. I'll look into attached issue a bit more.

@ShikaSD
Copy link

ShikaSD commented Nov 30, 2023

I ran a quick profile on the clustering sample with 1000 items. I think the new strategy that changes the viewport should help quite a lot, as it creates less items in general.

For Compose clusters, majority of the time is spent in creating and setting up AndroidComposeView instances to draw them on canvas. This is known to be slighly expensive as we need to setup wrappers for composition locals and other things, and certainly something we should optimize on our side as well. I also cannot help but wonder if there are better ways of rendering part of composition into a bitmap without involving a view per item, probably something worth optimizing for us well.

@wangela wangela added priority: p2 Moderately-important priority. Fix may not be included in next release. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. needs more info This issue needs more information from the customer to proceed. labels Dec 15, 2023
@wangela
Copy link
Member

wangela commented Dec 15, 2023

Can anyone verify that using NonHierarchicalViewBasedAlgorithm resolves your high-volume clustering performance issues? @mbilalhussain96 I see in your original post that you were already specifying NonHierarchicalViewBasedAlgorithm, but your specifications for width and height differ from the demo app which specifies metrics.widthPixels / metrics.density for width and metrics.heightPixels / metrics.density for height.

cc @ColtonIdle

@googlemaps googlemaps deleted a comment from Willkxxx Dec 15, 2023
@kikoso
Copy link
Collaborator

kikoso commented Dec 15, 2023

@mbilalhussain96 , could you eventually share your implementation of MarkerClusterRenderer?

@ColtonIdle
Copy link

For anyone that was curious of how to grab height and width from compose code, you can see a discussion I brought up in kotlinlang slack: https://slack-chats.kotlinlang.org/t/16243087/what-would-be-the-compose-way-of-grabbing-these-window-value#eb72b9b4-d4a9-462d-bbd7-c727dfed29d3

@LouisCAD
Copy link

LouisCAD commented Jan 9, 2024

TL;DR is use Modifier.onSizeChanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

6 participants