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