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 optional count argument to Semaphore::post #93605

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

RadiantUwU
Copy link
Contributor

@RadiantUwU RadiantUwU commented Jun 25, 2024

Implements a new optional argument in Semaphore.post to allow resuming multiple threads at once.

@RadiantUwU RadiantUwU force-pushed the thread-barriers branch 2 times, most recently from 0076121 to 96f75a8 Compare June 25, 2024 18:56
@RadiantUwU RadiantUwU marked this pull request as ready for review June 25, 2024 18:56
@RadiantUwU RadiantUwU requested review from a team as code owners June 25, 2024 18:56
@Chaosus Chaosus added this to the 4.x milestone Jun 25, 2024
@Chaosus Chaosus changed the title Add optional count argument to Sempahore::post Add optional count argument to Semaphore::post Jun 25, 2024
core/core_bind.cpp Outdated Show resolved Hide resolved
core/core_bind.cpp Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor

Mickeon commented Jun 25, 2024

May I ask in what occasion would this be useful? It is a relatively innocent change, but it's still worth knowing why it would need to be done.

@RadiantUwU
Copy link
Contributor Author

It's just so you dont have to do a for loop anymore to resume more and more threads, and it would be way more optimized than constantly locking and unlocking the mutex, adding 1 to the count and running notify_one.

core/core_bind.compat.inc Outdated Show resolved Hide resolved
core/core_bind.compat.inc Outdated Show resolved Hide resolved
core/core_bind.compat.inc Show resolved Hide resolved
core/core_bind.cpp Show resolved Hide resolved
core/core_bind.cpp Show resolved Hide resolved
core/core_bind.cpp Outdated Show resolved Hide resolved
core/core_bind.h Show resolved Hide resolved
core/core_bind.h Show resolved Hide resolved
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Leaving my approval on the added API. This needs to have the compatibility stuff fixed (which I'm not very familiar with) fixed per @AThousandShips comments before merging.

@Bromeon
Copy link
Contributor

Bromeon commented Jun 26, 2024

Was a named method considered (post_n or something)? Not sure what's the usual API design in such cases.

@RadiantUwU RadiantUwU force-pushed the thread-barriers branch 2 times, most recently from e62d00d to de4386b Compare June 26, 2024 15:30
core/core_bind.h Outdated Show resolved Hide resolved
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Is count as the name of the parameter completely fine? Yyyyeah, I'm sure it'll be fine.

doc/classes/Semaphore.xml Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

The co-author additions for just reviews aren't necessary by the way, feel free to remove them

Co-authored-by: RandomShaper <[email protected]>
Co-authored-by: A Thousand Ships (she/her) <[email protected]>
Co-authored-by: Mickeon <[email protected]>
@Mickeon
Copy link
Contributor

Mickeon commented Aug 27, 2024

I think we can safely push this to 4.4 . Still waiting on the GDExtension and DotNet teams for review.

@akien-mga akien-mga merged commit 8ae2c3a into godotengine:master Aug 27, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@RandomShaper
Copy link
Member

I've found something a bit concerning... One of my PRs was tested against 4.3, where it worked. On master it broke. The reason was that on master I had to add .bind(1) to a Callable to core_bind::Semaphore::post(); otherwise, I would get too-few-args error. On 4.3, adding the .bind(1) makes the code fail with too-many-args error. Therefore, there's a compat breakage here. Or is the reason that we have an issue with the binding-call system not honoring the default arg in case the .bind(1) is missing?

@AThousandShips
Copy link
Member

The callable_mp system doesn't know anything about bound methods so it can't honor the default argument, and in c++ default arguments are a kind of decoration and won't actually be relevant to the method pointer itself, so it's not trivial to handle that without somehow utilizing the method bind system

@AThousandShips
Copy link
Member

AThousandShips commented Sep 17, 2024

It's been discussed (forget where) and I've looked at some tricks to make it easier by creating a custom callable from a MethodBind but haven't done much on it as it's a bit convoluted, and would require fetching the method (also wouldn't work with non-bound methods, though we could use the bind without using the ClassDB system)

But will see about opening some issue to track it and look at ideas when I can

@RadiantUwU RadiantUwU deleted the thread-barriers branch September 17, 2024 12:42
@Mickeon
Copy link
Contributor

Mickeon commented Sep 17, 2024

If this is a severe problem reverting this PR should be fine as it's a relatively small Quality-of-Life feature.

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.

7 participants