godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.12k stars 69 forks source link

Add shared lock/unlock functions for Mutex #7423

Open ghsoares opened 1 year ago

ghsoares commented 1 year ago

Describe the project you are working on

A game with procedural terrain generation

Describe the problem or limitation you are having in your project

In my project I'm procedurally generating the terrain with noise and race track, the noise is always constant from the start of the game, but the race track is also procedurally generated to add segments relative to the player.

In that case I'm adding the segments in a separate copy and then pass the segments data to the actual track data used by the terrain generation, which is generating chunks in another thread.

This would lead to data racing, which I could fix using mutexes to read/write operations, but while the race track is only written once a while, each chunk reads a lot of times the track data, once per vertice, and the mutex only allows one reader or one writer, this makes the chunk generation process slower.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The proposal is to add shared lock and unlock functions to the Mutex class, so multiple threads can own the mutex lock, alongside the current functions which only one thread can own the mutex lock. This allows to create read/write logic where multiple threads can read some data at the same time, while only one can write at a time, blocking other write or read locks while the exclusive lock is active.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Basically, when you want to execute a block of code which writes data:

mutex.lock()
# write some data
mutex.unlock()

And then when you want to read data:

mutex.shared_lock()
# read some data
mutex.shared_unlock()

This way, multiple threads can read the data at same time, but only one can write and block reading.

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, it would be needed to be inside core.

Is there a reason why this should be core and not an add-on in the asset library?

Same as above.

AThousandShips commented 1 year ago

Would require changing the underlying Mutex to be a shared type, which probably has worse performance when used non-shared, I'd suggest adding a new SharedMutex for this

Changing the main Mutex wouldn't be accepted as there's no shared and recursive mutex to use

ghsoares commented 1 year ago

I see, in this case I'd suggest adding a class called RWLock which would serve the same purpose, while having a better semantics for it's intended use case, having [try_]write_[lock/unlock] and [try_]read_[lock/unlock]. In the core source code, there's already a class with same name (RWLock), maybe adapting into a separate class for Godot's API would work?

AThousandShips commented 1 year ago

Will take a look at making a bind for it, but the risk is that it's non-recursive so need to handle the undefined behaviour if exposing it to scripting

RadiantUwU commented 4 months ago

Hello, I am looking to pick this up.

RadiantUwU commented 4 months ago
class_name RWLock extends RefCounted

var _mtx := Mutex.new()
var _exc_mtx := Mutex.new()
var _sem := Semaphore.new()
var _thr_await := 0

func lock_exclusive() -> void:
    _exc_mtx.lock()
    _mtx.lock()

    while _thr_await > 0:
        _mtx.unlock()
        _sem.wait()

func try_lock_exclusive() -> bool:
    if _exc_mtx.try_lock():
        _mtx.lock()
        if _thr_await == 0:
            _mtx.unlock()
            return true
        _mtx.unlock()
        _exc_mtx.unlock()
    return false

func unlock_exclusive() -> void:
    _exc_mtx.unlock()

func lock_shared() -> void:
    _exc_mtx.lock()
    _mtx.lock()
    _thr_await += 1
    _mtx.unlock()
    _exc_mtx.unlock()

func try_lock_shared() -> bool:
    if _exc_mtx.try_lock():
        _mtx.lock()
        _thr_await += 1
        _mtx.unlock()
        _exc_mtx.unlock()
        return true
    return false

func unlock_shared() -> void:
    _mtx.lock()
    _thr_await -= 1
    if _thr_await == 0:
        _mtx.unlock()
        _sem.post()
    else:
        _mtx.unlock()

Just like in the pull request above:

Locking shared and then exclusively will cause a deadlock. Please avoid any situations like these.