search

KSPModdingLibs / KSPCommunityFixes

Community patches for bugs in the KSP codebase
50 stars 17 forks source link

Add caching to LocalizerPerf #229

Closed richardcocks closed 4 months ago

richardcocks commented 4 months ago

When profiling time-warping in the Tracking Station, I noticed an unsual amount of time spent generating contracts, and a large part of that was spent in KSPCommunityFixes.Performance.LocalizerPerf.Localizer_FormatStringParams_Prefix(string, string[], ref string)

It appeared to be repeatedly looking up the same basic translations as it calculated contracts each update.

This patch adds a very basic cache.

There may be more appropriate caching structures to use, this assumes that the cache won't grow too big.

richardcocks commented 4 months ago

Sorry, I accidentally targetted upstream. This was supposed to be a PR into my fork.

gotmachine commented 4 months ago

If I may, this is a very bad idea. That cache will definitely grow out of control due to usages where the params are a truly dynamic value. Beside, I couldn't reproduce the contract related overhead you describe in a stock install, so I would hazard to guess this might be caused by a mod ?

richardcocks commented 4 months ago

Hi @gotmachine,

Thanks for the feedback. This wasn't intentioned for this repo, I was going to keep this a private fork.

You're right that without cache ejection this will grow too large, given the large amount of dynamic parameters.

My intention is to add cache ejection so that only frequently-accessed templates and parameters hang around in the cache.

You're also correct about mods being the culprit of the localized contracts. Lots of mods add Contracts and each one adds to the localization burden.

The worst offender is ContractConfigurator.ScanSatCoverage, ( https://github.com/jrossignol/ContractConfigurator/blob/master/source/ContractConfigurator/Parameter/SCANsatCoverage.cs#L64 ).

It recreates contracts on update, every 5 seconds of real time or every 100 seconds of elapsed game time. When time-warping, that triggers every single update. ( Perhaps it'd be better if I just patched that mod to be both rather than either of those conditions instead. )