Optimization pass focused on foam code (saves about 30% of cpu usage I think) #76104
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
Turns out most of the cost of foam is the reagents it carries, and the varying effects they have. I'm doing my best here to optimize them without touching "user space" too much That means doing things like prechecking if we're gonna spawn on top of an existing decal (from glitter, flour, etc), and using that same proc to also avoid spawning on unacceptable turfs (I had to convert inheritance to a bitflag system to make this work, but I think that's ok since we want it imparative anyhow) It's actually nice for code quality too, since it lets me clean up code that was using raw locates and weird var pong. god I wish I had implied types man.
Holy shit reagent code is a lot. I'm doing a bunch of small things here. istype in init -> typecache, removing procs that are called once and loop over a list we JUST looped over (ph and the caching for reactions in particular) I am mainly trying to optimize copy_to here, since that's what foam spams As a part of this, I removed a pair of update_total and handle_reactions calls that were done on the reagents we are copying FROM I have no god damn idea why you would want to do that, but if anything is relying on the copy proc modifying the source, then that code deserves to break Speaking of, I cleaned up handle_reaction's main filter loop a lot, removed a lot of redundant vars and changed it from a full loop w tracker vars to an early exit pattern this meant using a loop label, which is unfortunate, but this is the fastest method, and it does end up cleaning up the code significantly, which is nice Oh also I made the required_other var function even if there is no atom attached to the reaction, since I don't see why it wouldn't This last bit is gonna get a bit esoteric so bear with me Failing calls (which are most of them) to handle_reactions are going to be fastest if they need to check as few reactions as possible One reagent in a reaction's required list is marked as the "primary", and thus gets to trigger checking it. We need all the reagents to react anyhow, so we might as well only check if we have one particular one to avoid double checking Anyhow, in order to make most calls the fastest, we want these reactions distributed as evenly as possible across all our reagents. The current way of doing this is just taking the first reagent in the requirements list and using it, which is not ideal Instead of that, lets figure out how many reactions each reagent is in, then divy reactions up based off that and the currently divvied reactions This doubles the reagent index count, and takes the most common reagent, water, from 67 reactions to I think like 22 Does some other general cleaning in reagent code too, etc etc etc
…n once (I feel like this element should take a trait source or something to make them potentially unique, it's too easy to accidentially override one with another
… reg/unreg. I only really used it because I liked the componentization, but it costs like 0.2 seconds off init alone which is really stupid, so let's just do this the very slightly harder way
…his is in the space of like 0.05 seconds kinda save so I can put it back if you'd like, the double loop just felt silly
Timer name generation involved a LOT of string shit, some in ways where the string only existed for a moment. This costs a good bit of time, and can be reduced with only minimal impacts on the end product, so let's do that
…s a waste most of the time, tho I would LOVE to analyze at compile time to work out if we care
Unused vars have 0 memory cost, and the ref and list lookup here is REALLY expensive, for both init and foam spreading. Saves 0.2s off a station flood on meta, and 0.17s off init
… Also fixes a bug where foam would travel under public access airlocks.
LemonInTheDark
requested review from
stylemistake,
ninjanomnom,
Fikou and
ExcessiveUseOfCobblestone
as code owners
|
does this all have to be bundled together? i was giving it a cursory look and the whole TGUI stuff got distracting while I was trying to look for all the changes to fluid code. would also be neater for stuff like tracking this kinda thing in milestones or easy-TM-reverts in case something yonks and we don't lose out on all those other performance gains indefinitely it's okay if it has to be in a mega-PR but i'd rather more atomicization? it's good work but it's a pain to have to keep scrolling hither and thither |
Yeah sorry I uh, I got all focused last night/today and just sort of lumped everything in together. If it helps I've got the commits neatly sectioned off, so if it's less of a workload thing and more inspecting different "classes" of changes those should be useful to you. |
|
that's fine then, but i think that having more split-out would probably be better for analysis-sakes? it's ultimately whatever though but it's just my personal preference at some point |
TheVekter
added
the
Performance
san7890
pushed a commit
that referenced
this pull request
Jun 22, 2023
<!-- Write **BELOW** The Headers and **ABOVE** The comments else it may not be viewable. --> <!-- You can view Contributing.MD for a detailed description of the pull request process. --> ## About The Pull Request [Reduces timer insertion cost by 80%](c9e5b28) Timer name generation involved a LOT of string shit, some in ways where the string only existed for a moment. This costs a good bit of time, and can be reduced with only minimal impacts on the end product, so let's do that. Includes a compile flag to flip it back if we ever have trouble in future. This is about 0.1s off init, since we do a lot of timer stuff then too [Removes STOPPABLE flag from QDEL_IN, moves it to a bespoke macro](e7a5d7f) Its a waste most of the time, tho I would LOVE to analyze at compile time to work out if we care ## Why It's Good For The Game I like it when we don't spend all of our cpu time just setting the name var on timers. that's good and not bad. This saves time fucking everywhere. 15% off explosions, 0.1 seconds off init, bunch of time off foam. it's just good. Cherry picked out of #76104 since that was too cluttered (sannnnnn) <!-- Argue for the merits of your changes and how they benefit the game, especially if they are controversial and/or far reaching. If you can't actually explain WHY what you are doing will improve the game, then it probably isn't good for the game in the first place. --> <!-- Both 🆑's are required for the changelog to work! You can put your name to the right of the first 🆑 if you want to overwrite your GitHub username as author ingame. --> <!-- You can use multiple of the same prefix (they're only used for the icon ingame) and delete the unneeded ones. Despite some of the tags, changelogs should generally represent how a player might be affected by the changes rather than a summary of the PR's contents. -->
san7890
pushed a commit
that referenced
this pull request
Jun 22, 2023
) ## About The Pull Request Unused vars have 0 memory cost, and the ref and list lookup here is REALLY expensive, for both init and foam spreading. ## Why It's Good For The Game Saves 0.2s off a station flood on meta, and 0.17s off init. More time in other qdel heavy areas Pulled off #76104 for the sake of cleanliness
github-actions
bot
added
the
Merge Conflict
|
This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself |
github-actions
bot
added
the
Stale
jlsnow301
reviewed
Jul 6, 2023
tgstation-server
removed
the
Merge Conflict
Jolly-66
added a commit
to TaleStation/TaleStation
that referenced
this pull request
Jul 18, 2023
…0% of cpu usage I think) (#6862) Original PR: tgstation/tgstation#76104 --- Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com>
dwasint
referenced
this pull request
in dwasint/Monkestation2.0
Aug 22, 2023
<!-- Write **BELOW** The Headers and **ABOVE** The comments else it may not be viewable. --> <!-- You can view Contributing.MD for a detailed description of the pull request process. --> [Reduces timer insertion cost by 80%](tgstation/tgstation@c9e5b28) Timer name generation involved a LOT of string shit, some in ways where the string only existed for a moment. This costs a good bit of time, and can be reduced with only minimal impacts on the end product, so let's do that. Includes a compile flag to flip it back if we ever have trouble in future. This is about 0.1s off init, since we do a lot of timer stuff then too [Removes STOPPABLE flag from QDEL_IN, moves it to a bespoke macro](tgstation/tgstation@e7a5d7f) Its a waste most of the time, tho I would LOVE to analyze at compile time to work out if we care I like it when we don't spend all of our cpu time just setting the name var on timers. that's good and not bad. This saves time fucking everywhere. 15% off explosions, 0.1 seconds off init, bunch of time off foam. it's just good. Cherry picked out of #76104 since that was too cluttered (sannnnnn) <!-- Argue for the merits of your changes and how they benefit the game, especially if they are controversial and/or far reaching. If you can't actually explain WHY what you are doing will improve the game, then it probably isn't good for the game in the first place. --> <!-- Both 🆑's are required for the changelog to work! You can put your name to the right of the first 🆑 if you want to overwrite your GitHub username as author ingame. --> <!-- You can use multiple of the same prefix (they're only used for the icon ingame) and delete the unneeded ones. Despite some of the tags, changelogs should generally represent how a player might be affected by the changes rather than a summary of the PR's contents. -->
Kapu1178
added a commit
to Kapu1178/daedalusdock
that referenced
this pull request
Apr 17, 2024
Kapu1178
added a commit
to DaedalusDock/daedalusdock
that referenced
this pull request
Apr 22, 2024
* iterative explosions * enable camera shake * fixes * microopt * remove warn * linter * cleanup * fix semi-infinite loop * fix + r walls have high explosion block * fuck * fix log * tweak explosion smoke * fixes to throwing * eh fuck it * oops * begin exploding/stop exploding * fix proc ref * eh, i like tyhis better * devastating explosion human interaction fix * fixup humans * tweak human ex act * more changes to human ex act * fluid smoke * tgstation/tgstation#73555 * part of tgstation/tgstation#76104, bother if perf is an issue * fire prio stuff * whoops * fade in smoke * paranoid * boom boom shake the room * bomb cap * explosion throw refactor * runtime * eh
Absolucy
referenced
this pull request
in Absolucy/Monkestation
Jun 12, 2024
…215) ## About The Pull Request Unused vars have 0 memory cost, and the ref and list lookup here is REALLY expensive, for both init and foam spreading. ## Why It's Good For The Game Saves 0.2s off a station flood on meta, and 0.17s off init. More time in other qdel heavy areas Pulled off #76104 for the sake of cleanliness
dwasint
referenced
this pull request
in Monkestation/Monkestation2.0
Jun 13, 2024
* Fix several SHOULD_NOT_SLEEP hits in SStgui.update_uis() (#75411) There are several esoteric code paths that lead to `winexists` calls or sleeps in `ui_data`. This proc is meant to be a command without sleeping. See #75232  * Moves ui references from the tgui subsystem to datums themselves (#76215) ## About The Pull Request Unused vars have 0 memory cost, and the ref and list lookup here is REALLY expensive, for both init and foam spreading. ## Why It's Good For The Game Saves 0.2s off a station flood on meta, and 0.17s off init. More time in other qdel heavy areas Pulled off #76104 for the sake of cleanliness * Fixes some spurious runtimes in SStgui procs (#76251) ## About The Pull Request src_object is NOT guaranteed to be passed in. HHHHHHHHHHH Closes #76249 * Fixes tgui_open_uis tracking (#77101) Fixes tgstation/tgstation#77097 This breaks uis for everyone with fancy-tgui off and in cases where the window is closed by external action (byond closing it) which is pretty bad. --------- Co-authored-by: Jordan Dominion <Cyberboss@users.noreply.github.com> Co-authored-by: LemonInTheDark <58055496+LemonInTheDark@users.noreply.github.com> Co-authored-by: AnturK <AnturK@users.noreply.github.com>
5 tasks
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.
About The Pull Request
Foam is crummy at high load rn, both because it runs on a low priority background subsystem, and because it wastes a bit of time.
Let's reduce usage (while speeding up a bunch of other stuff too), and give it more cpu generally.
Optimizes reagent processing somewhat
Turns out most of the cost of foam is the reagents it carries, and the varying effects they have
I'm doing my best here to optimize them without touching "user space" too much
That means doing things like prechecking if we're gonna spawn on top of an existing decal (from glitter, flour, etc), and using that same proc to also avoid spawning on unacceptable turfs (I had to convert inheritance to a bitflag system to make this work, but I think that's ok since we want it imparative anyhow)
It's actually nice for code quality too, since it lets me clean up code that was using raw locates and weird var pong.
god I wish I had implied types man
Optimizes foam spreading in its most accursed aspect, reagent copying
Holy shit reagent code is a lot.
I'm doing a bunch of small things here. istype in init -> typecache, removing procs that are called once and loop over a list we JUST looped over (ph and the caching for reactions in particular)
I am mainly trying to optimize copy_to here, since that's what foam spams
As a part of this, I removed a pair of update_total and handle_reactions calls that were done on the reagents we are copying FROM
I have no god damn idea why you would want to do that, but if anything is relying on the copy proc modifying the source, then that code deserves to break
Speaking of, I cleaned up handle_reaction's main filter loop a lot, removed a lot of redundant vars and changed it from a full loop w tracker vars to an early exit pattern
This meant using a loop label, which is unfortunate, but this is the fastest method, and it does end up cleaning up the code significantly,
Which is nice
Oh also I made the required_other var function even if there is no atom attached to the reaction, since I don't see why it wouldn't
This last bit is gonna get a bit esoteric so bear with me
Failing calls (which are most of them) to handle_reactions are going to be fastest if they need to check as few reactions as possible
One reagent in a reaction's required list is marked as the "primary", and thus gets to trigger checking it.
We need all the reagents to react anyhow, so we might as well only check if we have one particular one to avoid double checking
Anyhow, in order to make most calls the fastest, we want these reactions distributed as evenly as possible across all our reagents.
The current way of doing this is just taking the first reagent in the requirements list and using it, which is not ideal
Instead of that, lets figure out how many reactions each reagent is in, then divy reactions up based off that and the currently divvied reactions
This doubles the reagent index count, and takes the most common reagent, water, from 67 reactions to I think like 22
Does some other general cleaning in reagent code too, etc etc etc
Fixes runtimes from the forced gravity element being applied more then once
I feel like this element should take a trait source or something to make them potentially unique, it's too easy to accidentally override one with another
Removes connect_loc usage in atmos_sensitive, replaces it with direct reg/unreg
I only really used it because I liked the componentization, but it costs like 0.2 seconds off init alone which is really stupid, so let's just do this the very slightly harder way
Micros foam code slightly by inlining a LinkBlockedWithAccess call
This is in the space of like 0.05 seconds kinda save so I can put it back if you'd like, the double loop just felt silly
Changes how foam processes slightly
Rather then treating spreading and processing as separate actions, we do both in sync.
This makes foam fade faster when spreading, which is good cause the whole spread but unclearing foam thing looks silly.
It also avoids the potential bad ending of foam spreading into itself, backwards and forwards. This is better I promise.
Bumps fluid priority closer to heavy eaters, moves it off background
Also fixes a bug where foam would travel under public access airlocks.
Why It's Good For The Game
Saves a lot of cpu just in general, from both init and live.
In theory makes foam faster, tho I'd have to test that on live at highpop to see if I've actually succeeded or not. Guess we'll see.