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

Add TaggedScheduler, couple lifetime of tasks and orchestrator #2398

Merged
merged 34 commits into from
Jun 28, 2024

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented May 17, 2024

This PR refactors the scheduler and thread pool architecture by coupling scheduled tasks with an opaque identifier intended to represent the owner of the submitted task. This enables queries on the thread pool based on tasks submitted by a specific owner, which is relevant as the same background thread pool can be shared among multiple map instances. Knowing what tasks are scheduled by a specific owner allows us to remove any timeout conditions when waiting on all submitted work to be completed. This should result in more consistent and deterministic behavior of background tasks in multiple map scenarios.

Copy link

github-actions bot commented May 17, 2024

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.4% +67.1Ki  +0.4% +64.0Ki    TOTAL

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

@mwilsnd mwilsnd changed the title Add TaggedScheduler, couple liftime of tasks and orchestrator Add TaggedScheduler, couple lifetime of tasks and orchestrator May 17, 2024
Copy link

github-actions bot commented May 20, 2024

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0010         -0.0008             0             0             0             0

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

Copy link

github-actions bot commented May 20, 2024

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.2%  +329Ki  +0.2% +64.5Ki    TOTAL

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

Compared to d387090 (legacy)

    FILE SIZE        VM SIZE    
 --------------  -------------- 
   +21% +24.7Mi  +411% +24.6Mi    TOTAL

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

@louwers
Copy link
Collaborator

louwers commented May 31, 2024

Should probably undo #2455

@TimSylvester
Copy link
Collaborator

Should probably undo #2455

It'll disappear in the merge, that code was removed.

@mwilsnd mwilsnd self-assigned this Jun 10, 2024
@mwilsnd mwilsnd added the bug Something isn't working label Jun 10, 2024
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.

Very clean.

@louwers louwers enabled auto-merge (squash) June 28, 2024 16:35
@mwilsnd mwilsnd disabled auto-merge June 28, 2024 18:18
@louwers louwers merged commit c4c7545 into maplibre:main Jun 28, 2024
34 checks passed
@westnordost
Copy link
Collaborator

If there will be a new release or beta/alpha release with this change, we could test if it solves #2410

@louwers
Copy link
Collaborator

louwers commented Jul 3, 2024

@louwers louwers deleted the scheduler-lifetimes branch July 3, 2024 15:15
@westnordost
Copy link
Collaborator

Thank you, I already added it to the branch and asked if it can still be reproduced: streetcomplete/StreetComplete#5693 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants