[Scummvm-devel] ATTN Porters: SaveFileManager API Adjustments

[Johannes Schickel]

On 03/20/2016 04:54 PM, Marcus Comstedt wrote:
> Johannes Schickel <lordhoto at scummvm.org> writes:
>
>> This is behavior you suggest for listSavefiles is exactly the reason
>> why I looked into this. Our default implementation thought this is a
>> good idea and did it. And that is a real problem, because it results
>> in discovered save files which can not be loaded.
> Huh?  How is that possible?  listSavefiles should only return names
> of files which actually exist.  If the file exists it should be
> possible to load it, if you use its correct filename (which is the one
> returned by listSavefiles).

*If* you use its correct casing in filename. And here is the issue how 
to do that in practice.

> If there is a file "FnorD.99" and listSavefiles is called with the
> glob "fnord.##", then listSavefiles should return "FnorD.99", because
> that is the name of the file which matches the glob, and the engine
> should pass the name "FnorD.99" to the openForLoading function, becase
> that is the save file that was discovered.  If the name of the file is
> "FnorD.99" and you call openForLoading with the name "FnorD.99" it
> should work regardless of whether the filesystem is case insensitive
> or not.

Let's look at the reality of things. Engine's usually have a load/write 
operation based on slot numbers. For opening the apprioriate save file 
they generate the save file name without caring about case issues.

This is arguably bad behavior on case sensitive file systems. According 
to you (and I agree here) good engine behavior is to use exactly the 
case returned by listSavefiles to avoid problems.

How would engines go on for implementing this? Naively: Use just 
listSavefiles to determine the casing for the specific slot number and 
then use it for openFor*. And this is exactly what you argue against 
because this can lead to bad performance.

>> So, how big would the penalty be for your system?
> Well, let's try to math it out.  Let's say that reading the file list
> and reading a small save takes the same amount of time T.  Let's also
> say that you have N saves which match the current target.  To present
> the save game dialog, the engine has to call listSavegames once,
> and openForLoading N times.  listSavegames only needs to fetch the
> list of files, to it takes time T.  openForLoading will open the file,
> which internally needs to fetch the file list to get the start block
> number, and then the file can be read, so T+T=2T time for that.
>
> So with current (case sensitive) implementation this takes T(2N+1),
> which amounts to maybe 5 seconds for a reasonable N.  Incidentally, it
> used to take T(2N+100-N) because the engine would war-dial all
> possible savegame names, which was horrible of course.  Hence the
> existence of listSavegames.
>
> Now, if each call to openForLoading needs to perform an extra file
> listing so it can figure out the case before it can call the real open
> function, the time for reading the file becomes 3T instead of 2T.  So
> the savegame dialog would now need T(3N+1) time to pop up, which is an
> increase of approximately 50%.  When it already takes several seconds,
> this is pretty bad.
>

I am a bit confused by your maths to be honest, openForLoading (with 
system casing) should only take T because it only lists the files once? 
So in total, for system casing it should be (N+1)T and for case 
insensitive (2N+1)T?
But in general what you argue against is exactly what you end up with if 
you leave the case underspecified?

Let's look at it again. What is our main goal? Be user friendly and have 
a smooth save/load experience for the user.

1) What can affect this experience? Messed up casings on case sensitive 
file systems.

2) Where can this come from? (In random order:) 1. Users copy over save 
files from friends and ignore case differences. 2. Copying over betweens 
systems which have different cases. 3. ScummVM messing up cases in 
previous ScummVM versions (i.e. trying to start a game with a different 
case variation of a target name or the user messing up target name case 
in the Edit Game dialog).

Result) To be on the safe side, every engine will need to implement case 
insensitive save file operations.
Please be aware that just because some issues can't happen on your 
system, doesn't mean the engine side doesn't won't have to worry about them.

Let's look at this specific use case of save/load dialogs again (which 
is not the only use case for save/load, since there is, for example, 
also quick saves via hotkeys). Based on this I will present what I 
personally learned from thinking about it.
An engine implementing case insensitive access for the dialogs on a 
unspecified (and thus potentially case sensitive) system will need to do 
the following:

1) List all available save files via listSavefiles, this is a single 
operation taking time T.
2) For N save files, it will need to read the description to present it 
to the user.
     A system file opening operation takes time T to obtain the start 
block (according to you, let's just go with it). And some time D to 
extract the description. As I described above a naive implementation 
would do another listSavefiles to figure out the case to use beforehand. 
This is takes again time T.
     In total that is 2T+D for extracting the description for every save 
file, i.e. N(2T+D) in total.

In total this gives me T+N(2T+D) = (2N+1)T+ND for a naive implementation.

What is the really important point we learn here? That opening a file in 
a naive way will take 2T time for every file opening operation. 
Everywhere in the engine, in other use cases too. For example, hot key 
based save loading, load from launcher, etc.

Now, an engine can be smart, it can try to cache the save file list to 
prevent another listSavefile for every file opening operation in the 
engine, reducing openFor* again to T instead of 2T (This will bring ups 
back to (N+1)T+ND for the dialogs).
This work we need to do in every relevant engine and in all places, 
which are all if we want to strive for the best user experience possible.

So in total we learned:
1) All engines will most likely do case insensitive save file operations 
for maximum user friendlyness.
2) Engines can improve performance by caching save file names and bring 
it back to the level of case sensitive access.

 From 2 we know that we can feature fine performance on case sensitive 
systems when putting effort in this. However, because of 1, this might 
be logic spread across all engines. A better idea is to have all this 
logic concentrated in one place: SaveFileManager. This makes it a good 
idea to simply make SaveFileManager case insensitive. For implementors 
of a backend it seems easy to use caching of the file list to improve 
speed of operations if necessary.

// Johannes