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

Asynchronous GeometryTile deletion #2051

Merged
merged 30 commits into from
Feb 28, 2024
Merged

Asynchronous GeometryTile deletion #2051

merged 30 commits into from
Feb 28, 2024

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented Jan 25, 2024

General performance augmentation. Defers destruction of a GeometryTile's mailbox to a worker thread. These tiles hold OpenGL objects which must be deleted on the correct thread, so those objects are deleted on the render thread.

A number of special concerns around synchronization had to be addressed.

The primary performance problem centers around the mailbox needing to take both mutex locks before it can be properly closed. As this was done on the render thread, it would stall the whole renderer when the map was in motion.

This started in #1928 and has since been moved to this development branch.

Copy link

github-actions bot commented Jan 31, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.5% +79.0Ki  +0.5% +80.0Ki    TOTAL

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

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: Patch coverage is 56.54952% with 136 lines in your changes are missing coverage. Please review.

Project coverage is 58.38%. Comparing base (003c062) to head (937e261).

Files Patch % Lines
src/mbgl/util/thread_pool.cpp 52.72% 2 Missing and 24 partials ⚠️
src/mbgl/text/glyph_manager.cpp 47.61% 0 Missing and 22 partials ⚠️
src/mbgl/tile/tile_loader_impl.hpp 48.48% 2 Missing and 15 partials ⚠️
src/mbgl/renderer/image_manager.cpp 33.33% 0 Missing and 14 partials ⚠️
src/mbgl/tile/tile_cache.cpp 50.00% 0 Missing and 14 partials ⚠️
src/mbgl/tile/geometry_tile.cpp 50.00% 1 Missing and 6 partials ⚠️
src/mbgl/util/thread_pool.hpp 70.83% 0 Missing and 7 partials ⚠️
src/mbgl/renderer/render_source.cpp 53.84% 0 Missing and 6 partials ⚠️
src/mbgl/renderer/tile_pyramid.cpp 58.33% 0 Missing and 5 partials ⚠️
src/mbgl/actor/mailbox.cpp 84.00% 3 Missing and 1 partial ⚠️
... and 10 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2051      +/-   ##
==========================================
- Coverage   58.38%   58.38%   -0.01%     
==========================================
  Files         572      574       +2     
  Lines       28136    28340     +204     
  Branches    11270    11376     +106     
==========================================
+ Hits        16428    16545     +117     
+ Misses       4158     4157       -1     
- Partials     7550     7638      +88     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Feb 1, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.5%  +741Ki  +0.3%  +104Ki    TOTAL

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

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +19% +22.2Mi  +401% +23.9Mi    TOTAL

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

Copy link
Collaborator

@alexcristici alexcristici left a comment

Choose a reason for hiding this comment

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

Looks great! The android benchmark runs very smooth now.

@mwilsnd
Copy link
Collaborator Author

mwilsnd commented Feb 2, 2024

!benchmark android

Copy link
Collaborator

@TimSylvester TimSylvester left a comment

Choose a reason for hiding this comment

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

The additional locks in the managers are still a potential performance issue, I think, but I don't feel like we should mess around with it more unless they are observed to be relevant.

I'd prefer to further reduce direct use of the Scheduler static variables to help make it easier to reason about what can and must be using the same thread pool, but not enough to keep this open.

@TimSylvester
Copy link
Collaborator

ok, I think this one is finally ready

@louwers louwers enabled auto-merge (squash) February 28, 2024 18:26
@louwers louwers merged commit 4fff8a5 into main Feb 28, 2024
29 of 31 checks passed
@louwers louwers deleted the async-geometry-tile-dev branch February 28, 2024 22:18
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

5 participants