On Tue, 1 Jul 2025 00:01:21 GMT, Shaojin Wen <s...@openjdk.org> wrote:
>> BufferedWriter -> OutputStreamWriter -> StreamEncoder >> >> In this call chain, BufferedWriter has a char[] buffer, and StreamEncoder >> has a ByteBuffer. There are two layers of cache here, or the BufferedWriter >> layer can be removed. And when charset is UTF8, if the content of >> write(String) is LATIN1, a conversion from LATIN1 to UTF16 and then to >> LATIN1 will occur here. >> >> LATIN1 -> UTF16 -> UTF8 >> >> We can improve BufferedWriter. When the parameter Writer instanceof >> OutputStreamWriter is passed in, remove the cache and call it directly. In >> addition, improve write(String) in StreamEncoder to avoid unnecessary >> encoding conversion. > > Shaojin Wen has updated the pull request incrementally with one additional > commit since the last revision: > > Revert "BufferedWriter buffer use StringBuilder" > > This reverts commit da902ca0b0bd6acc003deb8ad1ca0d6485a29a27. I agree with Roger - introducing new methods on JavaLangAccess to access the methods on String/StringBuilder and related core APIs is not the way to engineer the implementation of these APIs. I believe the whole exercise in this PR is premature, and it's not the first time either. The contributor opened a mail in the core-libs-dev mailing list, did not wait to hear from the people knowledegable in this area, went ahead and created a JBS issue, then created a PR and linked the PR to that issue which generated an official RFR inviting official reviews, when the entire idea is half baked. In all of this, the PR has zero regression tests added/referenced with the changes. In the mailing list discussion, the contributor was asked if they had looked into the tests to ensure whether there is enough coverage for these changes and what kind of testing might have been done https://mail.openjdk.org/pipermail/core-libs-dev/2025-June/148231.html. The response from the contributor is this https://mail.openjdk.org/pipermail/core-libs-dev/2025-June/148236.html which says that a new micro benchmark shows some percentage. I haven't seen any indication in the reply, stating what kind of tes ting has been done or what tests have been analyzed and why no new tests have been added. This isn't the first PR of such nature either, so I am not leaning towards giving the benefit of doubt that maybe the contributor misunderstood the question, I believe it's way past that point now. Changes like these require additional tests or a detailed analysis and explanation about which tests cover these changes and why new tests cannot be introduced. I believe pointing to new manually run microbenchmarks and claiming these are performance driven changes isn't appropriate. Some reviewers try and stay away from PRs like these for valid reasons. Some reviewers do allow some benefit of doubt to the contributors who are new (the current contributor isn't) and help/guide them in such PRs, and that's a good thing. But when PRs are linked against JBS issue and have generated a RFR, it becomes extremely hard to stay away from reviewing those when the proposed changes are going in a direction that isn't maintainable. Dragging in the JBS infrastructure and the official review process in these experimental changes wastes a lot of time, energy and patience. My suggestion is to put this PR into draft mode, edit the subject of the PR to remove the reference to the JBS issue so that it hopefully doesn't generate additional RFR mails and then go back to the original email discussion in core-libs-dev and seek inputs on how or whether these changes should be done. It's OK if that process takes a (very) long time, that's intentional for changes like these. It's also OK if the contributor doesn't have the time or doesn't wish to follow the slow nature of these discussions and then implementing those changes in a way which keeps it maintainable and adds and exercises the relevant tests. They can ask people familiar with this area whether the proposed idea merits creating a JBS issue and only then create one so that anyone else who is willing to follow these processes can pursue it further. ------------- PR Comment: https://git.openjdk.org/jdk/pull/26022#issuecomment-3031150436
