Okay so I think this is ready for review - I'm still slightly on the fence about including both metrics caching and the write buffer addition in 9.6. This definitely and without a doubt completely eliminates the difference in stackfile save speed between macOS and Windows; I also have a feeling it might significantly mitigate the issues with 'active' virus-scan technology too (although despite a lot of digging around that subject I have not managed to determine what exactly triggers them - but large numbers of WriteFile API calls seem to me, at least, to be a good guess).

The metric caching is easy to assess correctness of, and even if there was a flaw it wouldn't be a disaster (the worse that would happen is that you might get the wrong UI font, or a DPI calc would be wrong).

The other part - the write buffer addition needs more consideration - as if that is not correct (well at least as correct as before - see below) then that will cause stackfile corruption.

The current MCSystemFileHandle API is flawed in that it does not have the ability to return errors for some operations - the most critical being Close

This flaw has been in there forever (even before the MCSystemFileHandle interface was added!) and on all platforms and is due to MCS_close not signalling an error state - which it could with write-buffered file implementations (which all platforms are - even if not explicitly in engine code). The mitigation for this is to ensure that the stream is explicitly MCS_flushd after the last byte is written. So from that point of view this is now 'better' than before from a correctness point of view - assuming the write buffering is correctly implemented, that is.

The other two methods which cannot return an error are GetFileSize and Tell. In this case their return values are synthesized using the current write offset to give the value they would return if any unwritten data had been written. I don't think this synthesis makes any difference in practice.

@mwieder It isn't wise to post comments on code in PRs which are marked as WIP as they can vanish from view (somewhat arbitrarily at times!). We use st-git (http://www.procode.org/stgit/) a lot internally to ensure well organised commits... Unfortunately, this means commit hashes are never stable back to the target/base branch and this causes issues for GitHub's tracking of source-level comments. Comments in the PR conversation for WIP things is fine though (as usual) :)

@runrevmark no worries - I missed the WIP label there.

@runrevmark GetFileSize could return an incorrect value if the file was opened in update mode. Aside from that I don't see any other problems 😉

@runrevmark I think you need a call to MCS_flush in MCFilesExecWriteToStream or we risk a breaking change in behaviour. Ideally this would be under user control at some point.

@runrevmark are we worried about successfully creating a write buffer that can never be written when the file is only opened in read mode?

@montegoulding
I take it the flush would be to ensure writes done by 'write to file' are immediate (so can be seen by other processes)? I guess this would be wise we do open files in SHARE mode on Win32 (even for write). However, it would be a change in behavior on other platforms then (which buffer by virtue of stdio). Perhaps we should just disable the write buffering for all but stackfile save streams for now.

@montegoulding
We only create a write buffer if Write is called and it is not a pipe... So I'm not sure how one could be created for a stream which is only read? Streams opened for read are only ever read from, and those opened for write are only ever written to - there is a guard in the write syntax methods to check whether the target file has been open for read... Is there an example you can see where this might happen?

@livecodeian That is a very good point re update mode. Before changing the mechanism (I'll probably go for deferred error reporting) I'll double check whether GetFileSize is ever called when the stream is a stackfile write stream (as discussed above we probably want to limit the buffering to those types of streams for now).

@livecodeian I think I've resolved the file size issue - it now computes the filesize as the max of (filesize + buffered-write-size) and (fileptr + buffered-write-size). I've also mitigated when write buffering will occur - restricting it to when stackfiles are being saved.

@montegoulding I've made it so that write buffering is only performed when the file handle is created for a stream which is used for writing a stackfile so that should resolve the issue with the change in behavior on windows for 'write to file' syntax this would have caused otherwise.

To resolve the latter I've added a new file open mode BufferedWrite which does the same thing as Write on all platforms apart from Windows; but on windows enables the write buffer in the created file handle. Hopefully I've added the relevant checks in all the right places! The BufferedWrite mode is only used when opening a stream which is to be used to write a stackfile.

@runrevmark shouldn't that be max(actual_file_size, file_ptr + buffered_write_size) as the buffered_write_size won't affect the size unless it goes beyond the current EOF?

Alternatively you could just flush the buffer before getting the size

@livecodeian Yes - you are correct - think I've amended that now :)

From a comment made by @montegoulding offline, I double checked the use of Tell() and Seek() when saving stackfiles. It turns out that MCS_tell() and MCS_writeat() are both used. Therefore Seek and Tell have to be able to work correctly with pending data to write. This means I will have to move operation to always flushing on Seek and Tell, but deferring any reported error until the next operation which can return one.

Marking this as WIP for now.

I decided a modicum of caution was required here - so have split out the metrics caching into #7342. This needs a fair bit more work to be mergeable.