LightMind opened a new pull request #461: URL: https://github.com/apache/cordova-plugin-file/pull/461
### Platforms affected All ### Motivation and Context Writing large files with FileWriter consumes very large amounts of memory, because the underlying cordova.js function converts ArrayBuffers to base64 encoded strings by using String concatonation and using JSON.encode on all the data. [Link to issue](https://github.com/apache/cordova-plugin-file/issues/364) ### Description This PR aims to improve the conversion of ArrayBuffers to base64, by using FileReader.readAsDataUrl, which is much faster and more memory efficient. The PR also writes the data in chunks of 1MiB, because the cordova code calls JSON.encode, which again has problems with large strings. First, I refactored the code, to have more named and private functions, to make all the asynchronous calls more easy to follow. Then, in FileWriter.write, I check that we received an ArrayBuffer that should be written to disk. If so, we branch away from the existing code. Then chunk the ArrayBuffer, convert chunks to Blob's with `octet` encoding, turn them into Base64 encoded strings, and then write them one at a time. ### Testing I have used my plugin in our existing cordova Application, and have written both real and synthetic files up 200MiB, and checked that the written files have the correct content. I could open written PDFs and images just fine. For synthetic data, I checked that the output of FileReader.readAsArrayBuffer matches the input to FileWriter.write exactly. I measured writing speed and memory requirements with chrome's debugging tools. Write speeds have improved to a consistent 4-5 MiB/s on my old Nexus 5x. Memory requirements look much more reasonable and are linear, instead of having extreme spikes with the old version. See my analysis in the comments of [the linked issue](https://github.com/apache/cordova-plugin-file/issues/364) I have not tested on a real iOS device yet, but will be able to do so very soon. ### Checklist - [ ] I've run the tests to see all new and existing tests pass - [ ] I added automated test coverage as appropriate for this change - [ ] Commit is prefixed with `(platform)` if this change only applies to one platform (e.g. `(android)`) - [ ] If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct [keyword to close issues using keywords](https://help.github.com/articles/closing-issues-using-keywords/)) - [ ] I've updated the documentation if necessary ### Questions I hope it's ok to use the PR for further discussion. I am not sure, how the FileWriter should behave, if it encounters an error while writing chunks. I see two possibilities: * Try to revert everything that has been written for this invocation of .write(), i.e reset the position and delete the contents of the filer after that position, or * Leave the file as-is, and passing an error that tells the caller how many bytes were written up until the error. Introducing the chunked writing also makes we wonder; Should there be a parameter on FileWriter, that lets callers change the chunksize, or turn off chunking. Mostly for the case where a write should either succeed or fail completely. This PR should not be merged, until we figure out what to do here and I can update the tests and documentation accordingly. Also, there is a `tests` folder. Are there any instruction on how to run the tests in the browser / on a real device? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
