suspend threads in parallel #1110
Conversation
MartinNowak
force-pushed
the
parallelSuspend
branch
from
46f299d to
bbc751a
Compare
From a few ms (growing with the number of threads) to 100-200 us. |
MartinNowak
force-pushed
the
parallelSuspend
branch
from
bbc751a to
95403a1
Compare
|
It seems like you're doing more here than just suspending threads in parallel, which could be done by changing about 10 lines of code (I think there's even a comment to this effect in core.thread). Maybe this should be broken into multiple separate pull requests? |
Sure, I can split of the critical sections part into a separate pull. See #1116 |
MartinNowak
force-pushed
the
parallelSuspend
branch
from
95403a1 to
b4f0690
Compare
|
Now it's only a single commit remaining. |
|
This is supposed to be part of 2.067's GC speedups, so a timely review would be appreciated. |
|
Anyone? |
src/core/thread.d
Outdated
There was a problem hiding this comment.
This is not a change by this PR, but why isn't m_isInCriticalRegion not checked again when the thread is actually suspended? It could have been changed after the check here and before the actual suspension.
There was a problem hiding this comment.
Another optimization might be to also interleave checking the critical regions of the threads, though it is probably not used that much.
There was a problem hiding this comment.
It could have been changed after the check here and before the actual suspension.
It shouldn't and that was a bug introduced by the other pull.
See #1164 for a fix.
|
I'm not a Posix expert, but this looks good to me. I can merge tomorrow if there are no objections... |
MartinNowak
force-pushed
the
parallelSuspend
branch
from
b4f0690 to
a74a031
Compare
|
Let's wait with this until after 2.067. |
|
LGTM would be awesome to revive this. |
|
ping @MartinNowak |
|
It's still blocked by #1164. |
- it's not possible to add a thread while suspendAll holds slock
- so Thread.getThis will always be set in the signal handler
- heavily reduce suspendAll time with many threads by signalling all N threads in parallel and then waiting N times for the semaphore rather than doing that serially - won't improve anything on OSX/Windows which do use a synchronous suspend API - move critical region handling into suspend method - change FreeBSD's m_suspendagain workaround to a per thread flag
MartinNowak
force-pushed
the
parallelSuspend
branch
from
a74a031 to
277b5b2
Compare
|
Resurrected, works now that we found a nice solution to the suspend during startup issue. |
|
Anyone? |
|
All I can say is that the diff looks sensible just skimming it. I don't quite have the time to give it a proper review right now, unfortunately. Maybe @alexrp or @complexmath can chime in? |
|
Alex hasn't worked on D since about a year, and Sean only finds time very sporadically. |
|
Auto-merge toggled on |
suspend threads in parallel
|
We have full release cycle to test and anything to help GC pauses is top priority IMO. |
|
Thanks, the most critical part of that series was MartinNowak@7ec6520 anyhow. |
threads in parallel and then waiting N times for the semaphore rather
than doing that serially
suspend API