tgstation / dev-cycles-initiative

Hub for tracking issues in the Dev Cycles Initiative
1 stars 0 forks source link

GAGS is far too expensive #9

Open Mothblocks opened 1 year ago

Mothblocks commented 1 year ago

GAGS has to perform very expensive icon operations for every color variation that is needed. This creates a very nice interface, where you can just slot in icon, but it carries a huge weight on init times.

Worse, the cost of GAGS makes profiling the cost of anything related to GAGS harder to measure. More on this later.

Estimated Cost

/datum/controller/subsystem/processing/greyscale/proc/GetColoredIconByType is 1.46 seconds of init time.

Howver, 91% of calls are only 22ms. The rest are fairly scattered at the end. This is most likely the cache.

The bulk cost is simply in icon operations. GetPixel (1.19s cost, from /datum/greyscale_config/proc/GenerateBundle) especially is very slow, but because it is serving its purpose of forcing the icon to generate--any icon operation would be doing the same.

Solutions

It is possible to precompile statically-inferrable GAGS usages. As in, everything we can predict is going to use GAGS (like from constant greyscale_config and greyscale_colors values) can be put into a .dmi, pushing the costs away from init time. This would come with an extra compile time cost, but it could only be incurred when the inputs are different, like if a .dmi changes or a variable changes. Dynamic generation still exists, but we statically create as much as we can. There is a prototype of this in Rust, but we could even potentially have a DM backend for this and run dd.exe when it comes out to use it.

It is also potentially possible for objects to choose between generating icons and using something like overlays, when applicable. However this could carry non-negligible runtime costs from either adding the overlays, or maptick costs if vis_contents was used. It also is under the assumption the bulk of this cost is in these simple icons, which we have not yet proven.

Mothblocks commented 1 year ago
diff --git a/code/controllers/subsystem/processing/greyscale.dm b/code/controllers/subsystem/processing/greyscale.dm
index 0c0db7b4f70..ec2b7a5a537 100644
--- a/code/controllers/subsystem/processing/greyscale.dm
+++ b/code/controllers/subsystem/processing/greyscale.dm
@@ -34,6 +34,8 @@ PROCESSING_SUBSYSTEM_DEF(greyscale)
        configurations[i].Refresh(TRUE)

 /datum/controller/subsystem/processing/greyscale/proc/GetColoredIconByType(type, list/colors)
+   if (UNLINT(TRUE))
+       return 'icons/testing/greyscale_error.dmi'
    if(!ispath(type, /datum/greyscale_config))
        CRASH("An invalid greyscale configuration was given to `GetColoredIconByType()`: [type]")
    type = "[type]"
optimumtact commented 1 year ago

if you're going to prebuild gags stuff, don't guess, just change the framework such that you can statically infer everything

Mothblocks commented 1 year ago
  1. That's not possible because dynamic GAGS is something that is actively supported player-facing--we allow custom clothes and stuff like greyscale HUDs want to exist and be configurable
  2. That puts a lot more pressure on the system to be correct--right now a theoretical system could only statically infer half the consumers, and the rest would be a performance sink instead of broken
optimumtact commented 1 year ago

make it possible

itsmeow commented 3 months ago

https://github.com/BeeStation/rust-g/pull/13 https://github.com/BeeStation/BeeStation-Hornet/pull/10455

Reduces worst-case GAGS generation time from 250ms to 1ms