GFile: read/write with fa.*_buffer

[StatisMike]

• Moved to FileAccess::get_buffer() and FileAccess::store_buffer() for Read::read() and Write::write().
• Minor change to BufRead::fill_buf().
• Added Debug derive to NotUniqueError
• Removed redundant and unused file_tests.rs

Adresses #513

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-528

[Bromeon]

Please revert this deletion.

[StatisMike]

@Bromeon These were the duplicated tests from back in my #473 messy rebase. They were unused - tests from gfile_test.rs are used. Why should they remain?

[Bromeon]

Ah, I see! This looked like an accident. A short comment would have helped here 🙂

[Bromeon]

Thanks for the PR!

Out of curiosity, did you notice any speedups?

[andreymal]

Hmm, there's something strange: it's still slow, and my implementation is still ~25x faster.

To find out what's going on, I hardcoded the length (because I know the size of my file):

-let length = self.fa.get_length();
+let length = 8431328; // the evilest thing ever

...and got ~22x performance boost. 🤯

Is get_length sooo slow??

[StatisMike]

@Bromeon Tested with itest benchmarks with repeat=5 on 32 kB file:

New impl:

   gfile_test.rs:
   -- gfile_read                 ...    270.959μs    705.541μs
   -- gfile_write                ...   1704.544μs   2886.615μs

Old impl:

   gfile_test.rs:
   -- gfile_read                 ...  15761.553μs  22200.658μs
   -- gfile_write                ...  13042.582μs  18482.637μs

Edit: even now the speedup in gd-props crate is noticeable (though I also needed to modify its gdbin save/load methods).

New impl:

                                              min       median
   gdbin.rs:
   -- serialize                  ...     36.407μs    120.048μs
   -- deserialize                ...    104.800μs    249.089μs
   -- gdbin_save                 ...    615.600μs   1763.703μs
   -- gdbin_load                 ...    355.608μs   1293.882μs

   gdron.rs:
   -- serialize                  ...     43.996μs    156.973μs
   -- deserialize                ...    124.621μs    314.222μs
   -- gdron_save                 ...   1758.172μs   3510.704μs
   -- gdron_load                 ...   2240.602μs   3546.316μs

Old impl:

                                             min       median
   gdbin.rs:
   -- serialize                  ...     35.421μs    125.010μs
   -- deserialize                ...     94.926μs    233.228μs
   -- gdbin_save                 ...   1100.371μs   2368.553μs
   -- gdbin_load                 ...   6630.350μs   9962.413μs

   gdron.rs:
   -- serialize                  ...     53.596μs    165.240μs
   -- deserialize                ...    122.907μs    341.042μs
   -- gdron_save                 ...   4590.429μs   6492.873μs
   -- gdron_load                 ...   7267.605μs   9566.152μs

@andreymal Will test out if fa.get_length is absolutely necessary. I think the 22x performance boost is something to strive for :)

[Bromeon]

Thanks to both of you for the measurements! 👍

Godot docs also recommend the while file.get_position() < file.get_length() pattern.

Can we theoretically cache length? Writing would invalidate the cache.
Not sure about other open file handles (but if that were possible, it could also happen during Read impl).

[StatisMike]

@Bromeon Not only recommend, but actually practically enforces. I've copy-pasted the implementation provided by @andreymal and with out itest got test failed:

   -- read_trait_works ... ERROR: Rust function panicked in file itest/rust/src/engine_tests/gfile_test.rs at line 83. Context: itest `read_trait_works` failed
   at: <function unset> (godot-core/src/lib.rs:155)
ERROR: Panic msg:  couldn't read numbers: Custom { kind: Other, error: "GodotError: Error { ord: 18 }" }

The error 18 means "EOF".

So I think I need to go back to the writing board and cache the file length. Better than caching position, for sure :)

You can move open file in read/write mode and the file length could potentially change only during write, potentially checking the other conditions (stored value length > file lenght - cursor position) would be overshoot. Intermediate read and write operations are usually problematic, I think this strategy will be good either way.

As we already try to enforce unique reference to FA, I think if the user tries to open and write to the same file with different GFiles at the same time... errors are their responsibility.

@andreymal I see, the FileAccessPack::get_buffer shouldn't try to get bytes after EOF, so this error shouldn't be our problem. If it gets fixed we could completely omit the file length check!

[andreymal]

@StatisMike I found a bug in FileAccessPack::get_buffer, I'm filling the issue right now

[andreymal]

godotengine/godot#85921

(However, if itest doesn't use FileAccessPack, then there is probably another bug somewhere else, but other FileAccess implementations seem to be correct)

[StatisMike]

Strange, you didn't receive the error I pasted from itest results while using your implementation provided in #513 (comment) ? The failing test tries to read file to the end:

https://github.com/godot-rust/gdext/blob/master/itest/rust/src/engine_tests/gfile_test.rs#L81-L87

Edit: Addition of fa.eof_reached() doesn't fix it. Back to caching file length ;)

[andreymal]

@StatisMike reaching EOF after calling get_buffer is not an error, this is expected and correct behavior. This means that your check_error function is too "paranoid" in this specific case. The read function should not fail it if reaches EOF, it should just return zero, as described in its documentation.

(and yes, I didn't notice this in my implementation because my project never reaches EOF. Well, my bad 🙃)

(UPD2: I added if self.fa.get_error() != Error::ERR_FILE_EOF as a workaround in my implementation)

[StatisMike]

@andreymal Oh, the source of my mistake was the Godot docs that didn't point to ERR_FILE_EOF as not being an error: https://docs.godotengine.org/en/stable/classes/[email protected]#enum-globalscope-error. Modifying check_error() to allow this output makes it pass the tests - so the .check_length() is not necessary.

@Bromeon The problem with muting this error is that it is also emitted when trying to do any other operation after EOF. So I think we could do something like that:

    // Error handling utility function.
    fn check_error(&self, allow_eof: bool) -> Result<(), IoError> {
        let error = self.fa.get_error();
        if error == Error::OK || (allow_eof && error == Error::ERR_FILE_EOF) {
            return Ok(());
        }

        Err(IoError::new(
            ErrorKind::Other,
            format!("GodotError: {}", error),
        ))
    }

And passing true only in Read and BufRead implementations

[andreymal]

Damn, there is a warning in FileAccessMemory::get_buffer that ruins everything:

	if (read < p_length) {
		WARN_PRINT("Reading less data than requested");
	}

If we should assume that EOF is always an error, then we have to cache file length :(

[Bromeon]

@StatisMike Sounds good. What do you think about covering the different scenarios with one test case each?

@andreymal Maybe we should also ask about this in godotengine/godot#85921? It's a bit strange to me that unlike most other file APIs, you can't just "read until EOF" but need to know the file length in advance.

[StatisMike]

Ok, so coming back with results.

• I applied the file_length cache - although we could conditionally mute the EOF Godot error, seeing the inconsistencies in Godot its better to cache it. The speed difference is negligible vs the ignore file length scenario.
• the Writer optimization as stated by @Bromeon. I'm not really sure why writing with BufWriter<GFile> is actually slower in gdext basic implementation - it made change in gd-props. It's possible that underlying crates implementations are making better use of BufWriter.

I'm personally happy with results (especially the write optimization!).

gdext:

                                              min       median
   gfile_test.rs:
   -- gfile_read                 ...    168.443μs    395.683μs
   -- gfile_bufreader            ...    103.616μs    401.653μs
   -- gfile_write                ...    172.306μs    453.944μs
   -- gfile_bufwriter            ...    198.734μs    479.341μs

gd-props:

                                              min       median
   gdbin.rs:
   -- serialize                  ...     38.885μs    113.830μs
   -- deserialize                ...     88.170μs    262.763μs
   -- gdbin_save                 ...    511.392μs   1740.631μs
   -- gdbin_load                 ...    385.845μs   1250.634μs

   gdron.rs:
   -- serialize                  ...     52.449μs    161.235μs
   -- deserialize                ...    132.524μs    370.224μs
   -- gdron_save                 ...   1365.221μs   3000.041μs
   -- gdron_load                 ...   1818.770μs   4833.154μs

What do you think about covering the different scenarios with one test case each?

Currently there are test cases for basic write and read - as they are only exposing Godot's FileAccess methods I don't thing there need to be test cases for every exposed function - if something would break I think we will see it in there. Additionally, there are also cases for Read, Write, BufRead and Seek traits implementations - additionaly the BufRead case also checks the file length cache, as I open the test file, write lines into it, rewind then read them back. What would be your suggestion for other cases I should cover?

EDIT: I think that I could also add BufWriter and BufReader to the test cases - especially with BufWriter, I think the default one makes use of Write::flush()

[Bromeon]

Thanks for adding BufWriter/Reader tests. I think that's enough for now 🙂

[StatisMike]

@Bromeon Also added self.clear_file_length() to Write::write() implementation - it didn't occur to me before that it is necessary there too.

[Bromeon]

• I'm not really sure why writing with BufWriter<GFile> is actually slower in gdext basic implementation - it made change in gd-props. It's possible that underlying crates implementations are making better use of BufWriter.

Maybe related to this, we have an internal buffer in GFile with Self::BUFFER_SIZE bytes -- the BufWriter around it has its own, separate buffer with different size.

How do the two play together?

[StatisMike]

Internal buffer for GFile is only for BufRead, the two shouldn't collide. Could provide something of such functionality in the future, but given the amount of improvement already done in Read and Write implementations and customizability of generic BufWriter I think it is out of the scope for this PR.

Edit: If you don't find any other issues with this I think it could be merged. The BufWriter being marginally slower than Write in itest is no issue for me, probably stemming from naive benchmarking. When passed on higher level in gd-props (to rmp_serde::to_writer and ron::to_writer), BufWriter<GFile> performed better than GFile itself, so it's probably the matter of using it itself rather than GFile problem.

[andreymal]

GFile::read is now fast enough for my case 👍

Regarding EOF, I accidentally started whining in godotengine/godot-docs#8590 but don't know what's next

[StatisMike]

@andreymal thanks for your input in this PR!

[Bromeon]

Thanks a lot to both of you!