[MIRROR] Fixes race condition in Life() #844
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Fixes race condition in Life() (#79934) ## About The Pull Request This fixes the following runtime by adding sanity checking for `reagents`:  It can be null, which I presume is from the mob being qdeleted in the middle of a `Life()` tick but I'm not 100% sure of that. The check for `QDELETED(src)` happens after `handle_organs()` and `handle_dead_metabolism()`, which there was no protection against there being a null reagent var for either of those procs. I was debating moving the order of the procs around, but I decided against it because I think it may be this way for a reason (`organ.on_death()` gets called in `handle_organs()`), plus again I'm not 100% sure that the `reagents` is being nulled from qdeletion. ## Why It's Good For The Game Fixes a bug that kept coming up in CI ## Changelog :cl: fix: fixed a runtime in handle_dead_metabolism() /:cl: --------- Co-authored-by: MrMelbert <51863163+MrMelbert@ users.noreply.github.com> * Fixes race condition in Life() --------- Co-authored-by: Bloop <[email protected]> Co-authored-by: MrMelbert <51863163+MrMelbert@ users.noreply.github.com>
AnywayFarus
added a commit
that referenced
this pull request
Nov 27, 2023
Iajret
pushed a commit
that referenced
this pull request
Feb 8, 2024
* Refactors fancy type generation (#81259) ## About The Pull Request [Refactors fancy type generation](tgstation/tgstation@3f218ac) Ok so we have this proc that generates concatenated names for types so admins have a nice list to sort through. The trouble is this is done by, for each type, iterating all possible replacements, and seeing which ones apply (with expensive string operations) A clean run of this applied to all datums takes about 3.5 seconds on my pc. This sucks. Ok so can we do better. Well, yes, pretty easily. Rather then, for each potential type, iterating all the options, let's build a zebra typecache (a lookup list of type -> string to use), and use that. Then we can use a list of replacement -> the bit to tear out to figure out what to remove. This works quite well. It does mean that we're doing it based off the type tree and not type paths, so if we didn't have a replacement for like, mob, it'd look weird, but we don't have cases like that so it's fine. Or well we sorta did, didn't have anything for atom movables or areas, but I fixed that so sall good. Anyway, we only need to do this work once. It takes about 0.3 seconds on my machine, so we can cache it. Just this on its own would technically slow init, since we have a some code that's running this proc off static, but we can just not, that's fine (technically saves init time too since we don't have to burn 0.1 seconds on it anymore). This brings the cost of generating this list for all datums from 3 seconds to 0.16, assuming we have the static pre generated. We could in theory pre-generate just like, all the strings? But I don't think the cached cost is high enough for that to be a real problem. IDK open to other thoughts Oh also I had to reorder the strings in that list, cause zebra_typecacheof has reverse priority. s life [Updates stat tracking macro to work at world start](tgstation/tgstation@1fbfb70) It for some reason doesn't actually get anything this early, but now at least the logging would in theory function ## Why It's Good For The Game Better response times for admins, faster code, more better * Refactors fancy type generation --------- Co-authored-by: LemonInTheDark <[email protected]>
ReezeBL
pushed a commit
that referenced
this pull request
Feb 9, 2024
* Refactors fancy type generation (#81259) ## About The Pull Request [Refactors fancy type generation](tgstation/tgstation@3f218ac) Ok so we have this proc that generates concatenated names for types so admins have a nice list to sort through. The trouble is this is done by, for each type, iterating all possible replacements, and seeing which ones apply (with expensive string operations) A clean run of this applied to all datums takes about 3.5 seconds on my pc. This sucks. Ok so can we do better. Well, yes, pretty easily. Rather then, for each potential type, iterating all the options, let's build a zebra typecache (a lookup list of type -> string to use), and use that. Then we can use a list of replacement -> the bit to tear out to figure out what to remove. This works quite well. It does mean that we're doing it based off the type tree and not type paths, so if we didn't have a replacement for like, mob, it'd look weird, but we don't have cases like that so it's fine. Or well we sorta did, didn't have anything for atom movables or areas, but I fixed that so sall good. Anyway, we only need to do this work once. It takes about 0.3 seconds on my machine, so we can cache it. Just this on its own would technically slow init, since we have a some code that's running this proc off static, but we can just not, that's fine (technically saves init time too since we don't have to burn 0.1 seconds on it anymore). This brings the cost of generating this list for all datums from 3 seconds to 0.16, assuming we have the static pre generated. We could in theory pre-generate just like, all the strings? But I don't think the cached cost is high enough for that to be a real problem. IDK open to other thoughts Oh also I had to reorder the strings in that list, cause zebra_typecacheof has reverse priority. s life [Updates stat tracking macro to work at world start](tgstation/tgstation@1fbfb70) It for some reason doesn't actually get anything this early, but now at least the logging would in theory function ## Why It's Good For The Game Better response times for admins, faster code, more better * Refactors fancy type generation --------- Co-authored-by: NovaBot <[email protected]> Co-authored-by: LemonInTheDark <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mirrored on Skyrat: Skyrat-SS13/Skyrat-tg#25279
Original PR: tgstation/tgstation#79934
About The Pull Request
This fixes the following runtime by adding sanity checking for
reagents:It can be null, which I presume is from the mob being qdeleted in the middle of a
Life()tick but I'm not 100% sure of that.The check for
QDELETED(src)happens afterhandle_organs()andhandle_dead_metabolism(), which there was no protection against there being a null reagent var for either of those procs.I was debating moving the order of the procs around, but I decided against it because I think it may be this way for a reason (
organ.on_death()gets called inhandle_organs()), plus again I'm not 100% sure that thereagentsis being nulled from qdeletion.Why It's Good For The Game
Fixes a bug that kept coming up in CI
Changelog
🆑 vinylspiders
fix: fixed a runtime in handle_dead_metabolism()
/:cl: