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

Android/core: Couple a sequenced scheduler to the lifetime of a GeoJSON source #2182

Merged
merged 6 commits into from Mar 11, 2024

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented Mar 7, 2024

The Android SDK location indicator is constructed using a GeoJSON source. Animation events for things like the pulsing circle trigger a GeoJSON source update within the core. GeoJSONData gets created to perform some async work on a sequenced scheduler.

In the case described above, a thread will be requested from the sequenced scheduler. As almost nothing else uses it (GeoJSON and logging, really), it is almost certain all threads in the scheduler's ring buffer have been destructed. So the scheduler creates a new thread and returns it. GeoJSONData does its work, quickly, and is disposed of. That leaves the scheduler thread with 0 references and it too is destructed. This results in the library creating threads that live for less than a millisecond constantly while an animated location indicator is on the screen.

This PR couples a sequenced scheduler directly with the GeoJSON source, keeping the thread used alive for the duration of the source. By not constantly creating and destroying threads, any potential performance impact from that activity is eliminated.

Copy link

github-actions bot commented Mar 7, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0%    -816  [ = ]       0    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results-ios/pr-2182-compared-to-main.txt

Copy link

github-actions bot commented Mar 7, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0% -3.26Ki  -0.0% -2.18Ki    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2182-compared-to-main.txt

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +20% +23.1Mi  +406% +24.3Mi    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2182-compared-to-legacy.txt

Copy link
Collaborator

@louwers louwers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation pointer of GeoJSONSource is swapped out when new data arrives. Do you know why they chose that? Wouldn't it make more sense to keep the implementation around for the duration of the parent object, but replace the data? Then the scheduler could also be part of the implementation class without this problem.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Mar 11, 2024

The implementation pointer of GeoJSONSource is swapped out when new data arrives. Do you know why they chose that? Wouldn't it make more sense to keep the implementation around for the duration of the parent object, but replace the data? Then the scheduler could also be part of the implementation class without this problem.

It's to support both GeoJSONVTData and SuperclusterData, looks like. I'm guessing the possible scenario for this approach would be changing a GeoJSON source to either enable or disable clustering, without having to recreate the source.

I can probably move the sequenced scheduler into the base class GeoJSONData, which is currently just an abstract interface. That means the setGeoJSONData method will need to check for an existing impl and transfer ownership of the scheduler to the new one. I don't think that would actually be safe to do, however, as the FeatureConverter runs async in the background scheduler and also posts work to the sequenced scheduler. It may be possible for an async task to try and post work to a sequenced scheduler while the impl is being swapped on a different thread, meaning we'd also have to guard it with a mutex.

@louwers louwers enabled auto-merge (squash) March 11, 2024 15:29
@louwers louwers merged commit 4e9392e into maplibre:main Mar 11, 2024
29 checks passed
louwers added a commit to louwers/maplibre-native that referenced this pull request Mar 15, 2024
louwers added a commit to louwers/maplibre-native that referenced this pull request Mar 15, 2024
@louwers louwers deleted the geojson-thread-thrash branch March 25, 2024 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants