TeamPorcupine / ProjectPorcupine

Project Porcupine: A Base-Building Game...in Space!
GNU General Public License v3.0
484 stars 279 forks source link

[Discussion] LocalizationTable Background Thread? #1768

Open BraedonWooding opened 7 years ago

BraedonWooding commented 7 years ago

Now I know there is a PR on the downloader aspect but what i've found tricky when trying to improve the speed of things like loading/saving of the settings menu is the biggest speed problem is the ...LocalizationTable.SetLocalization(int) where it seems to take quite a considerable amount of time. I guess it's more of a discussion about its speed then anything else. It also has a few weird occurrences and some weird here and there bits. So what do we think?

NogginBops commented 7 years ago

Does the localization table load in all localization data? That would slow down lookup considerably, if we only had like the last few languages loaded or like the current language + English I suspect it would be a lot faster.

BraedonWooding commented 7 years ago

The localization table speed is dependent partly on the total languages from what I can see (it's a bit from x() to y() back to x() then to z() back to y()... so it's a little hard to follow if just quickly glancing), but what seems to be the problem is a cumulative effect for example it uses a GameObject.Find and GetComponent every time to get the localization loader... and it then seems to load all the localization of every file but just 'skips' over the ones that aren't the default language but it does maybe 80% of the processing so basically it seems it presumes every language file contains multiple languages or something like that it's a little weird... So I think that a background thread (and some cleanup) would more hide this processing rather than fix it? Though I don't really think any overhaul is a top priority right now which is why maybe a background thread could be useful.

bjubes commented 7 years ago

~I ran the profiler on the settings menu save, and im getting a bit of a different result. only 20% went to applying settings (eventually calling updatelocalizationtable). The majority of time spent for me was in JsonConvert both deserializing and serializing json for settings, about 60%~

BraedonWooding commented 7 years ago

That's weird, because if I run it with it not doing the localisation its within a second, else it takes a short while but under 3 s

bjubes commented 7 years ago

my bad. When you change the language and then save changes, that delay becomes evident. capture

however its worth noting that a lot of the delays seem to be ultimately due to log calls. I can't extend the image far enough to see the rest, but its possible a good percentage of the delay can be log spam

bjubes commented 7 years ago

Im not getting a lot of baseline lag so I can't tell how well this works or not, but try disabling debug logs by commeting out line 72 of localization loader

UnityDebugger.Debugger.Log("LocalizationLoader", "Loaded localization at path: " + file);

This reduced the time (again its hard to tell) for me, and the profiler now just shows that all the lag is comming from LogStringToConsole, with only 10% going to LoadLocalizationInDirectory

BraedonWooding commented 7 years ago

Ohh I see, there is also the LoadingLanguagesFinished? or is that where that line is coming from

bjubes commented 7 years ago

LoadingLanguagesFinished calls a callback CBLocalizationFilesChanged, so I don't think the profiler can track down what is actually being called, most of it is just has Action.Invoke_void_this all the way down the screen. maybe thats something else to look into, but if its only 167ms on other peoples machines as well then its not really worth optimizing.

This does however bring up the bigger point that log calls are causing performance issues even when they are toggled off and even when the game is built and not logging anything.

koosemose commented 7 years ago

Are you sure they're causing an issue when toggled off? With all channels toggled off, I only get the call to Debugger.Log taking .1% of the time in LoadLocalizationInDirectory (.33ms out of 20.41) in deep profiling, and I can't even see any noticeable effects when not deep profiling. And it should be even less in builds, though since it's such a small amount it would be pretty negligible in the scheme of things, since it should short circuit even sooner.

The most significant slowdowns I see are LoadLocalizationFile and LoadConfigFile in LocalizationTable, with most of the time in those of course being the actual ReadAllLines call.

With those being the source of the slowdowns, I'm not sure how well it would work to do in the background, I assume we would still need to halt gameplay, since most of it is reading in the strings that are used everywhere, so I'm not sure how well things would work if we're changing the strings in another thread, it seems that might cause some issues.

BraedonWooding commented 7 years ago

Yes I agree with those findings of slow downs. Yes that is a fair point, I actually feel it's fine and just kind of one of those things that will never really be a big problem but will just sit there. Honestly I'm hard pressed to find a game with localization that doesn't take a considerable (a second or two) amount of time when saving and even quite a few that don't have it take a decent amount of time. So I think we are fine in this case :D, I'll close the issue because it seems to be the consensus that it doesn't effect gameplay enough and the exact cause is more ambiguous.