I'd like to explore this issue further as I also don't think the flush and sync calls add any value, but do have a severe impact on performance.
To resurrect the discussion, I've attached a new patch with the following approach:
- create a temporary file
- write class bytecode to temporary file, no flush or sync
- close temporary file
- atomic rename of temporary file to class file name
It is different to previous patches in that:
- it applies cleanly to master
- it checks return value from File.renameTo
- it omits proposed File.mkdirs change as the current implementation is actually converting from an "internal name", where forward slashes are assumed (splits on "/"), to a platform specific path using File.separator. I'm not convinced that the previous patch is safe on all versions of Windows, and I think it's separate from the main issue here.
I opted for the atomic rename of a temp file to avoid leaving empty class files with a correct expected class file name in case of failure.
It is my understanding that this patch will guarantee that:
- when writeClassFile returns successfully, a class file with the expected name will exists, and subsequent reads from that file name will return the expected bytecode (possibly from kernel buffers).
- when writeClassFile fails, a class file with the expected name will not have been created (or updated if it previously existed).
Anything preventing the operating system from flushing its buffers to disk (power failure etc) might leave a corrupt (empty, partially written) class file. It's my opinion that that is acceptable behaviour, and worth the performance improvement (I'm seeing AOT compilation reduced from 1m20s -> 22s on one of our codebases, would be happy to provide more benchmarks if requested).
Would be grateful for feedback/testing of this patch.