[Scummvm-devel] File handling revamp

[Lars Persson]

Hi!
This sounds execellt! 
What I discovered when looking at the file performance issues for Symbian is that due to alignment stuff, even an 16 bit read from an engine boils down to two byte reads. 

So having a generic cache function would really be benefitial, since that is what I'm doing in the Symbian file backend right now, and that has only taken a couple of days to implement. 

Of course this will depend on what you want to achieve with the generic buffering, but I think thats better than relying on underlaying system calls which might or not be available.
Best regards
Lars

> From: max at quendi.de
> To: scummvm-devel at lists.sourceforge.net
> Date: Tue, 29 Jul 2008 00:38:39 +0100
> Subject: [Scummvm-devel] File handling revamp
> 
> Hi folks,
> 
> as mentioned on this list and elsewhere a few times in the past, I  
> think our file handling system needs an overhaul. We already  
> introduced the FSNode stuff some time ago to try to improve the  
> situation with regards to abstracting the notion of paths, but that's  
> obviously not enough.
> 
> Let me list the problems I perceive right now, and some proposal as to  
> how to address this. As always, suggestions and constructive criticism  
> is highly welcome. Sorry, this mail is quite long, but that just shows  
> how bad I think the File handling code really is ... ;).
> 
> 
> * Lack of documentation, both for implementors/porters, and for users.
> -> Solution: Add more documentation :). I feel responsible for that,  
> but need help. In particular, I'll need people to read whatever I  
> fabricate (not just spelling, but also whether it is sufficiently  
> detailed). But of course everyone's welcome to contribute to the docs.
> 
> 
> * No good error handling, in particular, no clear error handling  
> semantics. We provide a bool File::ioFailed() method, and also an  
> eof() method, but we do not specify when exactly it will be set, and  
> by which methods.
> -> Solution: Adapt the ferror/clearerr semantics. They might not be  
> the best in the world, but they are already explicitly defined and  
> well-documented. People can just look at "man ferror", POSIX specs  
> etc. if something is not clear in our docs. Writing good doxygen  
> comments gets easier, too.
> 
> 
> * The overly convoluted, complicated, under-documented and abused  
> File::addDefaultDirectory() system (The blame is on me to a big  
> extent, of course :). Semantics are unclear here, too. Right now, we  
> blindly add every directory in our reach and it's friends, and then  
> just hope the files we want are included somewhere there. And that  
> there are no name clashes. No way of saying "game data files are in  
> these dirs, GUI theme stuff please only from that dir". One specific  
> problem: The AdvancedDetector's detectGame() function needs to support  
> taking either a FSList param, or alternatively uses File::open() to  
> locate files. Besides complicating the code, this causes a disparity  
> between detecting and running games, which can lead to subtle bugs.
> 
> -> Solution: Scrap it! And replace it with something better. Right now  
> I am not sure what *exactly* that means; we first will have to compile  
> a list of requirements. I.e., what exactly are those search paths used  
> for?. Then come up with a design proposal based on that.
> One approach might be to introduce a SearchDirectory class, which is  
> inited with a FSNode, and could cache the list of all files in it (and  
> optionally of its subdirs, recursively). Then either File::open  
> (optionally) takes (a list of) SearchDirectory object(s), and/or a  
> global set of active SearchDirectories could be stored. This approach  
> makes it easy to dynamically add/remove dirs from the search list  
> (just dump the SearchDirectory object); or to search for files only in  
> a subset of all search dirs. Not to mention that we get proper OO- 
> style code encapsulation.
> 
> 
> * Path separators (slashes) are silently creeping into File::open  
> calls, decreasing portability in a fashion that currently is hard to  
> detect.
> -> Solution: Change the code to use FSNodes as it should, or implement  
> the SearchDirectory proposal above, or something else which makes it  
> possible to conveniently load files from a (set of) dirs.
> Also: add a check to File::open() that warns (ultimately: errors out)  
> when a slash is used in a file name.
> 
> 
> * We have a very complex File::open method (did I already mention that  
> we don't document the semantics of stuff anywhere? ;).
> It has to do quite a lot: Handling search paths, trying to ignore  
> case, fall back solution for alternate file locations, etc.. This  
> leads to a very complex behavior, which can once more lead to subtle  
> bugs.
> -> Solution: Split it into two parts:
> 1) Locate the file, via search paths & directory FSNodes (which makes  
> it possible to add support for case insensitivity in a much more  
> logical and portable way), but also via port specific fallback options  
> (like for Mac OS X & iPhone right now).
> 2) Open the file. See also below.
> 
> 
> * On ports which do not support FILE, or where FILE is bugged, porters  
> have to use #define tricks to get class File to work. The DS, PS2 and  
> Symbian ports do just that, see common/file.cpp
> -> Solution: Make it possible to provide alternate File-class  
> implementations, just like for FSNodes. One way would be by adding a  
> FileManager class, with internal methods like
>    SeekableReadStream *openFileForReading(String name);
>    SeekableReadStream *openFileForReading(FSNode);
>    WriteStream *openFileForWriting(String name);
> and then the File class simply calls these in open(), and otherwise  
> calls-through to the stream object it obtained.
> 
> 
> * Right now, the default Savefile also makes use of FILE. Same  
> problems as above. We could change it to either use class File (which  
> we currently don't, to avoid the nastiness that is called File::open).  
> Or we could use the above FileManager class
> 
> 
> * Writing to files: This is not really a serious issue with our  
> current system, but something that contributes its share to the  
> complexity of class File and to the generic portability issues. I am  
> aware of the following reasons why we *write* data to files:
> 1) save states (handled via savefile managers)
> 2) config file
> 3) debug/resource dumps, screenshots, stuff like that
> 4) MT-32 emu cache; GUI theme cache; other cache files?
> Well, for 1) we already provide dedicated code (the savefile manager).  
> For 2) we badly need to provide dedicated code (see next point), and 3  
> is purely optional. So, in short, ports could let openFileForWriting()  
> return a dummy object which does nothing, if they don't support  
> writing to arbitrary files.
> 
> 
> * The config file right now is load & saved via class File. On the  
> Dreamcast, saving is disabled using #ifdef. The PS2 port generates a  
> fake special path to be able to handle the config file in a special  
> way. Maybe other ports to extra trickery as well.
> In addition, lots of ports provide special #ifdef'ed code to determine  
> the config file location. Nasty. Yet another place porters have to be  
> aware of.
> -> Solution: Add another set of FileManager methods:
>    SeekableReadStream *openConfigFileForReading();
>    WriteStream *openConfigFileForWriting();
> The default implementation could look like it does now, but the DC,  
> PS2 and other ports could easily customize it. No more complicated  
> tricks!
> 
> * File buffering: Recently some ports were bitten by the fact that  
> lots of our code rlies on file buffering. If you look at line 205 in  
> common/file.cpp, you'll also notice a
>      setvbuf(file, NULL, _IOFBF, 8192);
> (for __amigaos4__ only).
> Well, we should document this need; we should have such an setvbuf on  
> all systems where it is supported and makes sense. By letting backends  
> override the file code in a more flexible way, we hopefully also make  
> it easier to implement read buffering. If desired, we could even try  
> to provide some generic read buffering code.
> 
> 
> That's all for now. But I guess the list is long enough. Though I am  
> sure I forgot stuff. Porters, speak up if you can think of something  
> else that'd help! Engine maintainers and everybody else, speak up if  
> you have concerns missing on this list!
> 
> 
> Some more random rambling before I close:
> 
> * The more I think about it, the more I like the idea of having a   
> pure virtual FileManager class (to be implemented by ports, with a  
> default implementation based on FILE), which provides some  
> minimalistic core API for creating File/DumpFile/ConfigFile/SaveFile  
> streams. The point being that a port could use the exact same  
> implementation for all these types of files, or use a completely  
> custom one for each.
> Also, if we decide that we still need a global set of  
> SearchDirectories, then it might be useful to let the FileManager  
> manage them.
> 
> * We could even merge the SaveFileManager into the FileManager, though  
> I haven't really thought much about that, it's just a wild idea; the  
> current system works well, so I would only change it if it provides a  
> benefit to us, as such a change will force our porters to change an  
> otherwise working system.
> 
> * For those who haven't noticed: One goal I have in mind for ScummVM  
> is to move *all* port specific code into backends/, and not use any  
> #ifdef tricks in our main code base. This may not be 100% possible,  
> but I'd like to get as close as possible. It's just so much easier to  
> create a new port when you only have to look into backends/ to see  
> what all you have to do. Oh, and when things you have to implement are  
> properly documented.... ah, what a nice dream :)
> 
> * As a first immediate (and temporary) step, I propose to split the  
> reading and the writing part of our existing class File. The attached  
> patch does that. This is only  temporary, and simplifies the code in  
> class File a bit, enabling further refactoring. Whether we keep the  
> split between File and DumpFile or eventually merge them back together  
> remains to be seen, though right now I am in favor for keeping it.
> 
> 
> Cheers,
> Max
> 

_________________________________________________________________
News, entertainment and everything you care about at Live.com. Get it now!
http://www.live.com/getstarted.aspx
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.scummvm.org/pipermail/scummvm-devel/attachments/20080729/0016d84f/attachment.html>