[1.10.0] S.A.V.E - automatic backup system - 1.10.0-3173

[Lisias]

1 hour ago, micha said:

The proper fix is to figure out what's causing the exception and then fixing that, not randomly removing or disabling features/settings.

 

The problem is happening  when something wrong happens inside the thread while doing the backup, as a file not found, lack of privileges to read or write directories, etc. It's an error handler.

When you are logging only on KSP.log, no problems, things work fine. But when you activate logging on the screen, the windows process crashes because Unity's UI thread cannot access data from another thread.

On MacOS the thread just dies, and you can't shutdown the game normaly probably because the master thread tries to join a thread that's already dead. On Windows, unsurprisingly , it's sudden death for the whole process.

Then easy solution is just to do not log Exceptions on the Screen.

The proper solution is a special logging system that accepts logs from any thread and controllablily injects it on the UI thread (as we do on Java Swing!).

Edited July 1, 2020 by Lisias
tyops. as usulla.

[micha]

3 minutes ago, Lisias said:

But when you activate logging on the screen, the windows process crashes because Unity's UI thread cannot access data from another thread.

That makes sense, thanks. Fairly trivial to implement - logger writes to disk and, if log-to-screen is enabled to a queue, then the main thread's OnUI dumps whatever is in the error queue to screen.

Unity is fragile though.. "Handle With Care" LOL

[linuxgurugamer]

13 hours ago, Lisias said:

The problem is happening  when something wrong happens inside the thread while doing the backup, as a file not found, lack of privileges to read or write directories, etc. It's an error handler.

When you are logging only on KSP.log, no problems, things work fine. But when you activate logging on the screen, the windows process crashes because Unity's UI thread cannot access data from another thread.

On MacOS the thread just dies, and you can't shutdown the game normaly probably because the master thread tries to join a thread that's already dead. On Windows, unsurprisingly , it's sudden death for the whole process.

Then easy solution is just to do not log Exceptions on the Screen.

The proper solution is a special logging system that accepts logs from any thread and controllablily injects it on the UI thread (as we do on Java Swing!).

IMHO, the correct solution is not to use thread.  I've successfully done just that, and there is no performance impact in my current testing.

Also, I've made the following improvements:

• Replaced all threading code with CoRoutines
• Removed asynchronous setting
• Added SaveOnLaunch, with template string
• Rewrite config saves to standard ConfigNode format
• Added SaveConfirmationSound option
• Added QuickSave ability, with template string
• Added expiration limits to quicksaves
• Added tabs to configuration page to select between Backup Options, Quicksave Options and Sound Options
• Adjusted height of the Status and Clone pages to no more than 1/2 of the window height

I just finished the final part of the updated config, now need to do some testing in my big career install.

Given the changes, I'm thinking of calling my version something like S.A.V.E Enhanced

and one question for all of you:  Do you see a need to preserve the old config settings?

13 hours ago, Lisias said:

The problem is happening  when something wrong happens inside the thread while doing the backup, as a file not found, lack of privileges to read or write directories, etc. It's an error handler.

When you are logging only on KSP.log, no problems, things work fine. But when you activate logging on the screen, the windows process crashes because Unity's UI thread cannot access data from another thread.

 

Good to know the exact cause, thanks.

13 hours ago, micha said:

That makes sense, thanks. Fairly trivial to implement - logger writes to disk and, if log-to-screen is enabled to a queue, then the main thread's OnUI dumps whatever is in the error queue to screen.

Unity is fragile though.. "Handle With Care" LOL

Very, which is why, IMHO, the coroutine route is the safer way to go

[Lisias]

1 minute ago, linuxgurugamer said:

IMHO, the correct solution is not to use thread.  I've successfully done just that, and there is no performance impact in my current testing.

Also, I've made the following improvements:

• Replaced all threading code with CoRoutines
• Removed asynchronous setting

IMHO, these ones are not improvements. We will need to agree on disagree on this.

Co Routines inserts themselves on the critical path of the Game Engine, threads can be set to use only the sparing time of the CPU (not to mention better using CPUs with a lot of cores/hyper-threads).

Nereid had committed something on his repo 3 or 4 days ago, so the upstream is still alive. I will implement the fix suggested by @micha and apply a push request.

[linuxgurugamer]

26 minutes ago, Lisias said:

IMHO, these ones are not improvements. We will need to agree on disagree on this.

I agree to disagree.

The only changes he pushed was updating the project file.

And regarding coroutines, I'm using the WaitUntil to replace all the thread sleeps.  This way they impose zero impact on the game 99.99% of the time, and the backups are done so quickly that they still have minimal impact.

 

31 minutes ago, Lisias said:

 I will implement the fix suggested by @micha and apply a push request.

Hopefully he will do a release, but it's been 5 months since he was last on the forums.  I'll look at your changes to see how you do it.  I can always backfill the other changes I did onto the original codebase

[Lisias]

2 minutes ago, linuxgurugamer said:

the backups are done so quickly that they still have minimal impact.

On your machine.

There're people using shared memory GPUs, spinning disks and lower clocked CPUs. And I had some reports of somewhat more powered machines having to lower the FPS to 60 to prevent crashes inside the Unity.

 

4 minutes ago, linuxgurugamer said:

The only changes he pushed was updating the project file.

 Again, the project is not abandoned yet. I think it's premature to assume it's dead in the water.

Spoiler

Remember, we have a pandemic running wild on the World, people need time to cope with the new reality. A medic friend of mine are still quarantined, he got COVID19 last month and recovered well after 10 days, but the whole family is on quarantine yet.

 

[micha]

33 minutes ago, Lisias said:

Co Routines inserts themselves on the critical path of the Game Engine, threads can be set to use only the sparing time of the CPU (not to mention better using CPUs with a lot of cores/hyper-threads).

Oh?  I really need to learn more about Unity, I vaguely thought coroutines were basically a wrapper around threads.  But if they run on the main thread there's no way for them to take advantage of additional cores.  Just had a quick read and the main benefit seems to be that they can spread their execution across multiple frames. Good if there's some overhead left in each frame, not so good if the frames are already "full". In other words, highly dependant on single-core CPU speed + game load.

 

[linuxgurugamer]

Just now, Lisias said:

 Again, the project is not abandoned yet. I think it's premature to assume it's dead in the water.

Which is why I've just written him to find out his intentions.  I have no intent to fork the mod if he continues, if he does, then I'll just pull my stuff into a new mod.  I've offered to adopt it if he indeed is leaving, I have no intention to make an unfriendly fork.

1 minute ago, micha said:

Just had a quick read and the main benefit seems to be that they can spread their execution across multiple frames. Good if there's some overhead left in each frame, not so good if the frames are already "full". In other words, highly dependant on single-core CPU speed + game load.

Yes, you do have to be careful using coroutines.  It is possible for a single coroutine to lock up a game entirely.  They should be used for specific tasks only

[Lisias]

47 minutes ago, linuxgurugamer said:

Which is why I've just written him to find out his intentions.  I have no intent to fork the mod if he continues, if he does, then I'll just pull my stuff into a new mod.  I've offered to adopt it if he indeed is leaving, I have no intention to make an unfriendly fork.

That's where we have to disagree again.

I want you to have a fork and try to prove me wrong. It's how we discover things.

I just think it's premature to talk about adoption it on Forum.

[linuxgurugamer]

44 minutes ago, Lisias said:

I want you to have a fork and try to prove me wrong. It's how we discover things

I'm going to wait to hear from him, at least a few days

45 minutes ago, Lisias said:

I just think it's premature to talk about adoption it on Forum.

Which is why I wrote.  I had made an assumption which was wrong (that he was gone).

Anyway, enough on this.  We agree to disagree, I'll continue testing my version, and will start looking at 1.10

[linuxgurugamer]

I've pulled my code out of S.A.V.E. and have created a new mod for it, it's going to be called the AutoQuickSave System (AQSS).  Beta to be released later today or tomorrow

[linuxgurugamer]

For anyone interested, my Quicksave mod:

I've pulled it all out of S.A.V.E., and the only changes I have left in my local copy of SAVE are:

• Replaced all threading code with CoRoutines
• Removed asynchronous setting
• Rewrite config saves to standard ConfigNode format
• Adjusted height of the Status and Clone pages to no more than 1/2 of the window height
• These two were in a PR I submitted more than 1 1/2 years ago:
  • Added GUI.DragWindow if fixedWindowFloating was true
  • Fixed bug where excludes list was being cleared
• Moved windowPos from settings.cfg into main settings file
• Added option to set the window position

For now, I'll keep these local to avoid any unexpected versions in the wild.  If @Nereid does reappear, I'll offer to do a PR, and will even undo the threading to coroutine change if that's what he wants.  @Lisias I'd be interested in seeing your change to fix the crash issue, could you please let me know when you have a PR submitted? 

 

Edit:  I've just heard from him, he is around, and hopes to be back in the next few weeks

Edited July 2, 2020 by linuxgurugamer

[Nereid]

I have updated it to 1.10.0 but I didn't fix anything regarding the asynchronous backups.

If a crash only happens when the ingame debug window is open, I would not call this a major issue, because for debugging you could disable this setting anyway (and its off by default).

@linuxgurugamer: is there a way to to force an execution in the UI Thread? In Java Swing there are methods like invokeAndWait or invokeLater.

The asynchronous option is not there to improve performance but to keep I/O out of the UI-thread. But maybe there is a better way to achieve this in Unity.

 

[linuxgurugamer]

16 minutes ago, Nereid said:

@linuxgurugamer: is there a way to to force an execution in the UI Thread? In Java Swing there are methods like invokeAndWait or invokeLater.

@Lisias seemed to know more about this than myself, he seemed to think it would be fairly easy to do, and said in a previous message above that he would do it and push a PR to you.

I preferred to convert it to CoRoutines.  I am aware of the issue, but in my main game last night, my version with CoRoutines was able to backup 11 saves in less than a second.  Obviously it will depend on the computer speed, disk speed, number of and size of files, etc.  

I'd be happy to push my PR to you, but would prefer to wait to see if @Lisias is able to get his PR to you.

I do see an issue with the thread version, ignoring the io issue.  It's rather minor, what if a thread tries to backup a save that is currently being updated by the game?  You may have already addressed this, I don't think it's a big issue, if it is one at all

[Nereid]

@linuxgurugamerI'm already trying to implement a way to log in the main-Thread myself. It doesn't seem to be that hard.

6 minutes ago, linuxgurugamer said:

I do see an issue with the thread version, ignoring the io issue.  It's rather minor, what if a thread tries to backup a save that is currently being updated by the game?  You may have already addressed this, I don't think it's a big issue, if it is one at all

As far as I remember I prevent this by halting the game if this occurs. 

[Nereid]

Try 1.10.1-3185. It shouldn't crash even with async backups anymore.

[Lisias]

3 hours ago, Nereid said:

If a crash only happens when the ingame debug window is open, I would not call this a major issue, because for debugging you could disable this setting anyway (and its off by default).

It's due the log being called from another thread. As you said, it's like Swing - only the main thread can access the Widgets. I think it's am overlook from Unity, threading is going to be very important now that CPUs are doubling the cores instead of doubling the speed.

 

3 hours ago, Nereid said:

@linuxgurugamer: is there a way to to force an execution in the UI Thread? In Java Swing there are methods like invokeAndWait or invokeLater.

Yes, there's. It's essentially what I'm doing, but in a smarter way. Stay tuned.

invokeAndWait, however, is essentially squashing the thread to the main one - it will tax the main thread in a similar way as Co Routines would.

Ideally, a invokeLater mechanism is more performatic.

 

4 hours ago, Nereid said:

As far as I remember I prevent this by halting the game if this occurs. 

Halting the game I think it's a bit harsh, perhaps a popup dialog at the end of the process, showing a report of the whole process when the user is on a Scene where the popup would not interfere with the game? (opening a dialog when the user is executing a suicidal burn would render some compliments directed to your parents!)

 

2 hours ago, Nereid said:

Try 1.10.1-3185. It shouldn't crash even with async backups anymore.

Yep, that's it.

Your way is way simpler than the thing I'm implementing, but I think "my way" will be faster and more versatile. Now I have two more solutions to benchmark with my idea to see what will work better!

 

4 hours ago, linuxgurugamer said:

but would prefer to wait to see if [when] @Lisias is able to get his PR to you.

Here, I fixed that for you.  

[linuxgurugamer]

@Lisias

I wonder how many people will understand your identifier:

Quote

Location: Universe ! Virgo ! Milkway ! OrionArm ! SolarSystem ! Earth ! America ! SouthAmerica ! Brazil ! SãoPaulo ! Capital ! Home ! LivingRoom ! MyChair

Took me a minute to recognize it because of all the lines it was on

[micha]

10 hours ago, Nereid said:

@linuxgurugamerI'm already trying to implement a way to log in the main-Thread myself. It doesn't seem to be that hard.

As far as I remember I prevent this by halting the game if this occurs. 

The way to do it is with mutexes, but yes, it would halt the main thread while the second thread is busy writing the data to disk.

If the main thread execution speed is of paramount importance, one approach is to always have the second thread at a lower priority.  Then when it gets asked to perform the (lengthy) operation, it grabs the mutex, takes a copy of the data [1], releases the mutex, then performs the operation.  You always want to reduce the amount of time that a mutex is locked.

So in this case, the memory copy is relatively fast. In comparison, disk access is magnitudes slower, even on SSD. By working with a copy of the data, the actual write to disk can occur over many many frames without impacting the game.  It will only use cycles on another core, or any left-over cycles in each frame.

 

[1] That, of course, depends on the operation.  In this particular case that's an option. But if the worker thread needs to modify the data then it's often not possible.  Depending on the algorithm, it can still be improved through a combination of read and write locks, rather than the "hammer" of fully exclusive locks all the time. Using separate Read and Write locks allows for interleaving of operations, assuming that makes sense for the given task.

Edited July 6, 2020 by micha

[Nereid]

On 7/7/2020 at 12:25 AM, micha said:

The way to do it is with mutexes, but yes, it would halt the main thread while the second thread is busy writing the data to disk.

Under normal circumstances: yes. But in this case a simple volatile boolean and a "spin" to wait should suffice (it's a rare occasion anyway, and checking a boolean every 100ms won't cost much ).

Problem with mutexes in C# is, that you can't aquire them mutiple times and release them just once (bad idea, I already tried this). And I'm a bit relecutant to overdesign this, because it uses a blocking queue for the jobs and you can't aquire the mutex or a lock (which is  sufficient in this case) over the whole while loop that fetches the jobs. Aquiring the lock just for a single backup job is an incorrect solution either. Problem is: it's a while loop and I have to aquire the lock AFTER getting it from the queue and releasing it only when the queue is empty at the end of the loop. But maybe I have just not found the right class in C# to accomplish this task. I could do it myself writing my own class like in the old days of Java ;), but this is a bit over the top. 

[Yes, it's an excuse, because I'm to lazy to find a better solution or search for design patterns that solves this. ]

So I have to decide: Make it not just 99,999999999999999999...% safe and redesign the job handling or keep it as it is. 

In theory a new backup could be scheduled while there are still backups ongoing and just waiting to complete and KSP changes the files in between. But this is an academic problem, because new backups are just scheduled in the main menu or at the next save (which waits for the ongoing backups to complete). It just won't happen. 

Before the latest release there was a temporary solution implemented anyway (so async was not really async). I have just forgotten to fix this, but after I read my comment in the code...

And: I've never used the async option myself  and I never noticed any differences. II have just done it, because I'm used to avoid the UI thread for operations like this (never ever do any I/O in the event-dispatch-thread! - never ever!), but I'm unfamiliar with Unity.

EDIT: And you should never ever use mutexes in the UI-Thread either if they block because if I/O or other unpredictable reasons.

Edited July 10, 2020 by Nereid

[linuxgurugamer]

@Nereid

I'll probably be getting you a PR in a few days, with any fixes in my local which aren't in the current 1.10 branch.

 

Linuxgurugamer

Edited July 14, 2020 by linuxgurugamer

[Nereid]

7 hours ago, linuxgurugamer said:

@Nereid

I'll probably be getting you a PR in a few days, with any fixes in my local which aren't in the current 1.10 branch.

Feel free. I' still thinking about a cancel dialog if there are still backups ongoing. But is it realy worth the effort?

[Lisias]

On 7/9/2020 at 6:54 PM, Nereid said:

EDIT: And you should never ever use mutexes in the UI-Thread either if they block because if I/O or other unpredictable reasons.

We should never do any I/O on the UI-Thread, but yet Unity does. So if we have a lot of threads running, in need to do I/O that it's only safely done on the UI-Thread due the design of Unity (probably due legacy from the Unity 5 and earlier), you will need to take that hit.

The solution @micha gave above appears to be the best compromise.

-- -- -- POST EDIT 2020-1006 -- -- -- 

I WAS WRONG. Logging on Unity is thread safe, the dudes made it right.

It's KSP that messed up somewhere, somehow on the logging into the screen stunt.

In a way or another, some locking mechanism is still needed indeed - what happens is that locking mechanism need to be applied higher on the stack.

-- -- -- POST EDIT 2020-1008 -- -- -- 

Nope. Logging on Unity is thread safe on 2019. On 2017 and Unity5, it is not. (sigh)

Edited October 8, 2020 by Lisias
post edit

[linuxgurugamer]

8 hours ago, Nereid said:

Feel free. I' still thinking about a cancel dialog if there are still backups ongoing. But is it realy worth the effort?

I reviewed the code, the only thing not there is the floating window code, and that  isn't really important, unless some buttons leak through to the menu screen

[Lisias]

23 hours ago, linuxgurugamer said:

I reviewed the code, the only thing not there is the floating window code, and that  isn't really important, unless some buttons leak through to the menu screen

To tell you the true, I found this one pretty convenient - and since there's already an inactive option to activate it (instead of top, bottom, left or right), I would second including this code on the mainstream. You never know when a pesky add'on will popup a Warning over it.