Loading optimizations #269
Conversation
| gdb.progressTitle = "Loading model assets..."; | ||
| yield return null; | ||
|
|
||
| // call non-stock model loaders | ||
| modelsByUrl = new Dictionary<string, GameObject>(allModelFiles.Count); |
There was a problem hiding this comment.
Why not
modelsByUrl = new Dictionary<string, GameObject>(allModelFiles.Count, StringComparer.OrdinalIgnoreCase);
modelsByDirectoryUrl = new Dictionary<string, GameObject>(allModelFiles.Count, StringComparer.OrdinalIgnoreCase);
And the same for texturesByUrl?
It should have the same semantics as your existing equality logic, but probably will be faster based on your comment elsewhere about frequently falling through.
It might be slightly slower in the case where it's an exact match (since it has to do a tiny bit more work to compare), but not drastically - and it should eliminate fallback to the O(n) case for case insensitive matches.
I don't know if you still need the fallback (I saw in some cases that you might have to check a List because you could just directly modify it in stock KSP code for one of these optimizations), but thought I'd mention it.
https://learn.microsoft.com/en-us/dotnet/api/system.stringcomparer.ordinalignorecase?view=net-8.0
https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2.-ctor?view=net-8.0#system-collections-generic-dictionary-2-ctor(system-int32-system-collections-generic-iequalitycomparer((-0)))
There was a problem hiding this comment.
Model search is case sensitive, but yeah that could make sense for textures.
TBH, occurrences of the methods being called with mismatching casing were pretty rare in my tests (less than 1%), and for the few cases where it happens, the requested casing will be added as duplicate entry, so the overhead will only occur on the first call for a specific casing. The case insensitive comparer will still induce quite a bit of overhead, so I'm not so sure that would be a net gain overall.
Falling back to a linear search on the original stock lists is necessary in any case (:P), as (a few) models/textures are also loaded from asset bundles and because there nothing preventing external code from modifying the lists directly. That still leave a hole if someone decide to remove models/textures from them, but that feel unlikely enough.
On a side note, upon further review, we should probably also patch GetTextureInfoIn() and GetModel(), as those are commonly used too, including during part compilation.
There was a problem hiding this comment.
Fair - I assumed it was significantly more frequent based on this comment on the issue (which I happened across by chance when being curious about what the latest version you just published held :)).
"The dictionary approach is however not as beneficial here, because those methods allow to get a texture by its case-insensitive url, so we have to fallback to iterating on the list with a case insensitive string comparison if the requested url isn't correctly cased. The net result was still beneficial in my tests, but not as much as I hoped due to that slow path being taken relatively often."
InvariantCultureIgnoreCase and CurrentCultureIgnoreCase are fairly expensive; I'd be surprised (but would believe you) if OrdinalIgnoreCase was particularly expensive though. Then again, my knowledge may be more relevant for newer dotnet versions? (ex: https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-8/).
But fair, you're trading an initial miss + a bit of memory in an uncommon case, to save a bit of arithmetic / bit-twiddling + possibly other overheads on every case - I'm not a KSP modding expert, your intuition is likely better than mine in this area and you're in a position to measure it for sure :).
Pity you can't get rid of that silly list - I was gonna suggest "well if you can modify, why not binary search" - but that won't work if code makes additions without being aware of the need to insert at the right position.
There was a problem hiding this comment.
Yeah, unfortunately, the mono-derived BCL and JIT compiler used in KSP/Unity is missing all the juicy improvements from more or less the last 10 years (and in many cases, perform just plain worse than the good old .Net framework).
This being said, the case-insensitive dictionary still make sense and is a trivial change, so I will try and check what the profiling results have to say.
There was a problem hiding this comment.
Ah, right... I haven't touched Unity in about half a decade. My frame of reference for "old dotnet performance" is .net framework... I forgot that Unity has its own fork of Mono.
And I'm assuming Unity's version of Mono is just different enough / just tightly coupled enough that we can't e.g. just drop in a modern version of the dotnet runtime either... Though it might be fun to try sometime 😄
…e called manually. Main intended usage is for patches requiring to be applied before ModuleManagerPostLoad.
|
So, for reference, here are some profiling results regarding the GameDatabase methods : The texture getting methods were called 13 326 times during loading :
So all in all, I'd say that the difference is pretty negligible, but using the default comparer is either faster or just as fast. I haven't verified but I would guess the majority of the slow path cases happened in The real gains are for models anyway :
|
…e file / class. - Refactored those patches as a BasePatch using the new [ManualPatch] attribute (instead of patching manually with a separate harmony instance). - Patched a few additional texture/model getting methods.
I'd agree, thanks for sharing the numbers / satisfying my curiosity. And it's an interesting technique I hadn't though of for handling case insensitive lookup where usually the casing will match but it won't always. |
Note that this PR depends on / include #272
Various loading performance focused changes, part of the general loading re-implementation :
GameDatabase.GetModel*/GameDatabase.GetTexture*method to use them instead of doing a linear search.This was especially bad with models, as the method would compare the requested string to the
GameObject.nameproperty for every model in the database.Unfortunately, we have no way to know if additional assets are loaded by custom means, so we have to fallback to a linear search when the requested texture isn't known and we can't assume that a texture that isn't found at some point won't be found in the future, so in cases where a not loaded / absent texture is queried, there is no performance improvement.
Overall, this benefit to many scenarios, during initial loading for model parsing and part compilation, and more marginally for scenes switches when various initialization paths are re-acquiring a texture reference.
Other changes, also loading related :
MinorPerfTweakspatch, patched theFlightGlobals.fetchproperty to not fallback to aFindObjectOfType()call when theFlightGlobals._fetchfield is null, which is always the case during loading. In a stock + BDB test case, this alone was about 10% of the total loading time, 7+ seconds. The call doesn't seem necessary asFlightGlobals._fetchis set/unset fromFlightGlobals.Awake()/FlightGlobals.OnDestroy(). I guess there are some very edge cases where a (just about to be destroyed) instance could be acquired by a call toFindObjectOfType(), but in any case I would qualify such behavior as a bug. On a side note, the same issue is present withPhysicsGlobals, but I haven't been able to detect any benefits in patching that one.PartParsingPerfPartModule.OnIconCreate()and in some case, some model-manipulation code in Awake().Partfields parsing, by creating a dictionary of IL-emitted parser delegates instead of the very generic reflection based stock approach.Overall, these changes can provide a quite significant boost to loading time, mainly to part compilation and model loading. On a hot boot (config/model/texture loading not throughput limited by I/O), total time from exe launch to main menu for a stock + BDB install is now around 35 seconds on my 5800X3D.
As usual, this probably need to be tested a bit more before being released.