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

Take locks before signaling thread condition variables #2636

Merged
merged 2 commits into from
Jul 19, 2024

Conversation

mwilsnd
Copy link
Collaborator

@mwilsnd mwilsnd commented Jul 19, 2024

When notifying condition variables for the thread pool, it is possible that threads are just about to wait on the variable at the same time an external thread is about to notify. This can result in the notification being lost, and threads get stuck sleeping. Taking the condition locks prior to notifying prevents this from happening. This was most commonly observed with jobs sent to a sequenced scheduler which only has 1 worker thread.

@mwilsnd mwilsnd added the bug Something isn't working label Jul 19, 2024
@mwilsnd mwilsnd self-assigned this Jul 19, 2024
Copy link

Bloaty Results (iOS) 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  +0.0%    +104  [ = ]       0    TOTAL

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

Copy link

Bloaty Results 🐋

Compared to main

    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0% -1.48Ki  -0.0%    -428    TOTAL

Full report: https://maplibre-native.s3.eu-central-1.amazonaws.com/bloaty-results/pr-2636-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-2636-compared-to-legacy.txt

Copy link

Benchmark Results ⚡

Benchmark                                                     Time             CPU      Time Old      Time New       CPU Old       CPU New
------------------------------------------------------------------------------------------------------------------------------------------
OVERALL_GEOMEAN                                            -0.0047         -0.0041             0             0             0             0

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

@mwilsnd mwilsnd merged commit d15de92 into maplibre:main Jul 19, 2024
36 of 37 checks passed
@louwers louwers deleted the condition-variable-stalls branch July 19, 2024 21:41
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.

3 participants