Improve performance of muting/unmuting logic #3287
Conversation
This commit changes some of the muting/unmuting logic to improve performance. It combines some atomic load/store pairs with single add/sub atomic operations. It also removes an unecessary stack when unmuting actors that may have been muted due to sending messages to an overload or muted actor.
|
Assigning @SeanTAllen and @slfritchie because they are the experts here. |
|
I feel like there was a very important reason to not unmute until where it was done previously. Sadly, I didn't document it. I'd want to put this through a lot of runs of the ubench tests to verify that this doesn't result in any deadlocks. The existing code is the result of a ton of ubench runs where sometimes it took hundreds of runs to trigger deadlocks. |
This commit reverts the changed that removed an unecessary stack when unmuting actors that may have been muted due to sending messages to an overload or muted actor because it may be necessary for deadlock prevention.
|
@dipinhora that wasn't a request to remove the stack changes. more my musing that I couldn't remember why I did it but I seem to remember it being important. I was planning on spending some time trying to remember the "why". |
|
@SeanTAllen I've reverted the stack change. The other changes alone provide a significant performance improvement. We can revisit the stack change and ensuring it doesn't introduce deadlocks as part of a separate PR where we can ensure the appropriate testing is completed for the change. |
|
@dipinhora sounds reasonable. and man, i really wish i would have documented why that was there (or did it end up not being important? arg. bad bad sean). |
|
@SeanTAllen yes, i know it wasn't a request to revert them. I just didn't want to hold up the rest of the changes as they are not dependent on each other. |
|
@SeanTAllen I've created #3289 for the stack related change. |
| } | ||
|
|
||
| void ponyint_mute_actor(pony_actor_t* actor) | ||
| { | ||
| uint8_t is_muted = atomic_load_explicit(&actor->is_muted, memory_order_relaxed); | ||
| uint8_t is_muted = atomic_fetch_add_explicit(&actor->is_muted, 1, memory_order_relaxed); |
There was a problem hiding this comment.
interesting. i found i had this at one point when doing this work but it was changed later on. i wonder why.
|
@dipinhora i think i might remember why the stack is in there: If we unmute before then an actor could be rescheduled, send messages, get muted again and end up in more than 1 mute map. ** i think ** |
This commit changes some of the muting/unmuting logic to improve
performance. It combines some atomic load/store pairs with single
add/sub atomic operations. It also removes an unecessary stack when
unmuting actors that may have been muted due to sending messages
to an overload or muted actor.
The following are some microbenchmarks to measure impact of this change. Please keep in mind that they were run on a linux virtualbox vm on windows 10 system without cpu isolation and thermal cpu scaling/throttling was enabled.
Output of running master for
message-ubench:Output of running this PR for
message-ubench: