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

Reply via email to