Skip to content

Conversation

Taknok
Copy link
Contributor

@Taknok Taknok commented Jun 19, 2025

As ToSteam (used by XMLTransformer) writes char by char, using a buffered writer improves significantly the performance of the writing.

See #339 for the tests and the discussion

@LisoUseInAIKyrios LisoUseInAIKyrios changed the title Improve file writing perf: Improve file writing Jun 20, 2025
@kitadai31
Copy link

The JavaDoc of StreamResult says that you should use OutputStream usually.
https://docs.oracle.com/en/java/javase/17/docs/api/java.xml/javax/xml/transform/stream/StreamResult.html#%3Cinit%3E(java.io.OutputStream)

Normally, a stream should be used rather than a reader

So it should be it.outputStream().buffered().use {.

Moreover, you can use StreamResult(File f)
Neither the Writer nor the OutputStream is necessary.

@Taknok
Copy link
Contributor Author

Taknok commented Jun 20, 2025

StreamResult(File f) will force a buffered writer ?

@Taknok
Copy link
Contributor Author

Taknok commented Jun 20, 2025

it.outputStream().buffered().use { output ->
    TransformerFactory.newInstance()
        .newTransformer()
        .transform(DOMSource(this), StreamResult(output))
}

Will force a buffered path, but it does not differ from the approach

it.bufferedWriter().use { writer ->
    TransformerFactory.newInstance()
        .newTransformer()
        .transform(DOMSource(this), StreamResult(writer))
}

FileOutputStream is not buffered by default, so when you write to a FileOutputStream, every write() call can result in a system-level I/O operation, which is what it was asked to fix/avoid in this PR no ?

@kitadai31
Copy link

StreamResult(File f) will force a buffered writer ?

Apparently, it does not
https://github.com/openjdk/jdk/blob/33970629ac63eea6009fca7a34c8f333f1a60a37/src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java#L533
When I set a File to SrteamResult, it creates a FileOutputStream without BufferedOutputStream
Yes, you are right, thx

FileOutputStream is not buffered by default, so when you write to a FileOutputStream, every write() call can result in a system-level I/O operation, which is what it was asked to fix/avoid in this PR no ?

File.outputStream().buffered() is a kotlin extension function equivalent to BufferedOutputStream(FileOutputStream(file))

@Taknok
Copy link
Contributor Author

Taknok commented Jun 20, 2025

Which method would you prefer ReVanced to use — File.outputStream().buffered() or File.bufferedWriter()? From a system-level performance perspective, they are effectively equivalent.

@kitadai31
Copy link

I think ReVanced should use OutputStream according to the JavaDoc.

As ToSteam (used by XMLTransformer) writes char by char, using a
buffered writer improves significantly the performance of the writing.
@Taknok Taknok force-pushed the buffered-writer branch from da00f8a to 120add1 Compare June 20, 2025 09:38
@Taknok
Copy link
Contributor Author

Taknok commented Jun 20, 2025

Done

@oSumAtrIX oSumAtrIX changed the title perf: Improve file writing perf: Use a buffered writer to reduce IO overhead Jun 20, 2025
@oSumAtrIX oSumAtrIX merged commit 99f4318 into ReVanced:dev Jun 20, 2025
1 check passed
@Taknok Taknok deleted the buffered-writer branch June 20, 2025 13:27
github-actions bot pushed a commit that referenced this pull request Jun 20, 2025
# [21.1.0-dev.2](v21.1.0-dev.1...v21.1.0-dev.2) (2025-06-20)

### Bug Fixes

* Add back missing log by naming logger correctly ([#332](#332)) ([e4e66b0](e4e66b0))
* Support UTF-8 chars when compiling instructions in Smali in non UTF-8 environments ([#331](#331)) ([bb8771b](bb8771b))

### Features

* Use option name as key for simplicity and consistency ([754b02e](754b02e))

### Performance Improvements

* Use a buffered writer to reduce IO overhead ([#347](#347)) ([99f4318](99f4318))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants