[Scummvm-devel] ATTN Porters: SaveFileManager API Adjustments

Colin Snover scummvm-devel at zetafleet.com
Sun Mar 20 23:08:01 CET 2016


Hi,

I have been just peripherally following this discussion, so apologies if
this has been asked already. Since there seems to be a lot of talking
about performance, has anyone involved here actually done real-world
benchmarking to verify that there is actually a performance problem
caused by this change? If so could you please provide hard data about
what that actual performance impact is, when this change is made?

Thanks,

On 2016-03-20 15:28, Johannes Schickel wrote:
> On 03/20/2016 08:15 PM, Marcus Comstedt wrote:
>> No, doing listSavefiles _once_ to get the casing of _all_ saves does 
>> not lead to bad performance. It's performing the list operation _for 
>> each save_, which is what would need to happen if the functionality is 
>> supposed to exist inside openForReading, that leads to bad performance. 
> See below.
>
>> I included the time for actually reading the contents of the file
>> (needed to get the display name of the save for the dialog).
>> Technically, this happens already in openForLoading on the Dreamcast,
>> but that makes no practical difference.
> See below.
>
>> No.  If the engine passes the filenames to openForLoading that it got
>> from listSavefiles (which is enough for it to work witout specifying
>> the case mode), the backend can do an open on the lower level
>> directly, so it's
>>
>> Once:  list all files (listSavefile)
>>
>> For each save:  list files until a match is found (internally)
>> For each save:  read file data from the block location found in the
>>                  previous step
>>
>> versus (case insensitivity in openForLoading)
>>
>> Once:  list all files (listSavefile)
>>
>> For each save:  list files to find the correct case (case
>>                                          insensitivity emultion)
>> For each save:  list files until a match is found (internally)
>> For each save:  read file data from the block location found in the
>>                  previous step
>>
>> So it's  T(1+2N) versus T(1+3N) like I wrote.
> Why is reading data from the block a operation taking time T (i.e. time 
> to list save files + loading a small save in your scheme)?
> As I outlined, the way I see it is it takes (N+1)T+ND or (2N+1)T+ND, 
> where T is the time to list save files and D is the time it takes to 
> extract the description. I don't really see that T and D can be used 
> interchangably, nor what their relation is in general.
>
>> The user do not have to either care about or ignore the case.  They
>> just plug in the two memory cards, select the file to copy and the
>> card to copy it to, and it is copied.
>>
>> You donÃ't copy files betweensystems on the Dreamcast, you move the
>> memory card to the new system.
> Sure, this might not be an issue on Dreamcast. But engine logic working 
> on all supported systems has to do this anyway, even on Dreamcast. Thus, 
> engines worrying about it will also affect performance on your backend.
>
>> Changing target casing between versions sounds like a bad idea, yes. 
>> We should not do that. 
> We shouldn't. The user can. And there is not much we can do to avoid it.
>
>> I do not agree with that conclusion.  Using the filenames returned by
>> listSavegames should work for most engines.
>>
> See below.
>
>> Well, you call it "caching", but it's really just processing the list. 
>> It needs to go throgh the list one item at a time to populate its 
>> internal represenation of the savegame list. The item _is_ the 
>> filename, so it's right there, no caches needed. 
> See below. You need real caching for some use cases or suffer 
> performance issue there.
>
>> If there is really significant logic that would need to be replicated,
>> it could be put in common or something, but in general it would be
>> something like
>>
>>    for(savename : listSavenames)
>>       if (f = openForLoading(savename))
>>         addSaveToMenu(savename, f)
>>
> Not a single engine implements it like this. This suggests either they 
> assume it's case insensitive or they don't care.
> Since most of these engines have been for systems with case insensitive 
> file systems, it seems natural to assume we would feature the same 
> behavior to provide an easy environment to re-implement these engines. 
> This has been the case for Common::File.
>
> Saving the filename for available saves inside a dialog is not the only 
> issue we talk about here. We can re-work all engines with this use case 
> to work like you suggested. However, not in all cases listSavefiles has 
> been used before.
>
> Other cases include:
> 1) Quick save/load via hotkeys.
> 2) Loading from command line/launcher.
> 3) Using save file manager as only way to write non-save files which 
> store data for the engine. This is present also in our very first 
> engine: SCUMM, but, for example, also used in SCI.
> 4) Engines not using a listSavefile at all for save handling, for 
> example Gob (which is heavily original game script controlled IIRC), 
> Composer, MADE, and Sword25.
>
> Sure, we can probably put a lot of effort in it and implement all this 
> working fine on case sensitive systems. However, the common theme seems 
> to be that people want case insensitive access.
>
>> But beacuse of the way the API looks, the SaveFileManager does not
>> have access to the list generated by listSavefiles, that is owned by
>> the engine.  The backend could cache the list (in a more proper sense
>> of the term), but due to the lack of a session concept, it wouldn't
>> know when to discard said cache (this should happen when the dialog is
>> completely filled, to prevent stale entries).
> Are stale entries the only issue we talk about which prevent you from 
> caching available files inside your SaveFileManager implementation?
>
> // Johannes
>
>
> ------------------------------------------------------------------------------
> Transform Data into Opportunity.
> Accelerate data analysis in your applications with
> Intel Data Analytics Acceleration Library.
> Click to learn more.
> http://pubads.g.doubleclick.net/gampad/clk?id=278785231&iu=/4140
> _______________________________________________
> Scummvm-devel mailing list
> Scummvm-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/scummvm-devel


-- 
Colin Snover
https://zetafleet.com





More information about the Scummvm-devel mailing list