RFR: 8255729: com.sun.tools.javac.processing.JavacFiler.FilerOutputStream is inefficient [v2]

[Jonathan Gibbons]

On Sun, 3 Jan 2021 10:04:49 GMT, Joel Borggrén-Franck <jfranck at openjdk.org> wrote:

>> I read Joel's (@jbf) comment as understanding where you are coming from with the patch.
>> 
>> Since the code now provides the recommended pattern, I don't think any more evidence is needed. I think you're OK to integrate.
>
> Hi
> 
> In general when closing a performance related issue it is good to provide some kind of evidence that performance has actually improved. Many times I have seen fixes that *should* have solved some performance issue that turned out to not do that. That is why I still like to see some kind of evidence that this fixes the reported issue.
> 
> My second comment was about understanding how the code *could* solve the issue. After the explanation from @lgxbslgx I now believe this can be the case, it looks plausible that this will improve the buffering in writes. *However* the fact that I now think this could fix the issue isn't the proof I would look for when closing a performance bug.
> 
> I would still like to see some kind of performance metric or benchmark that shows that buffering of writes actually occur, this doesn't need to be a repeatable test, but can be a report or summary from an instrumented test run or something similar. However I do *believe* this should fix the problem so I don't object to Jon sponsoring this if he is confident with the change.

In general, I agree with @jbf about validating performance improvements. However, I think that this change is as much about cleanup as it is about performance, especially since `FilterOutputStream` specifically recommends that this method _should_ be overridden:

`
Note that this method does not call the write method of its underlying output stream with the same arguments.
Subclasses of FilterOutputStream should provide a more efficient implementation of this method.
`

It is not clear to be that we can write a reasonable performance test, since as has been noted, it depends on the underlying stream. But I do think that the proposed change makes the code cleaner and simpler, and in general will do no worse than before.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1854