Hi Roger,
> Are you taking into account that for many reads the data is not copied
> into the local buffer.
> See the comments in BufferedInputStream.read1: 277:280?
sure, I'm aware of this optimization. The buffer however is always allocated as
BufferedInputStream instantiated,
waisting by default 8 kB of memory.
If I take a benchmark [1] measuring costs of reading metadata of a simple
Spring bean it demonstrates [2]
measurable imrovement when buffering is dropped:
original
Mode Cnt Score Error Units
read avgt 100 122.041 ± 1.286 us/op
read:·gc.alloc.rate.norm avgt 100 50795.798 ± 13.941 B/op
<<
patched
Mode Cnt Score Error Units
read avgt 100 119.524 ± 1.171 us/op
read:·gc.alloc.rate.norm avgt 100 42635.578 ± 10.866 B/op <<
So in this case it's about 3 microseconds and 8 KB per bean.
When I measured the impact of change [2] on Spring Boot application I got
start-up time improved
from 760 milliseconds to 718 milliseconds a memory consumption decrease from
48,09 MB to 47,47 MB.
Also see https://github.com/openjdk/jdk/pull/2992
1.
https://github.com/stsypanov/ovn/blob/master/src/main/java/com/tsypanov/ovn/MetadataReaderBenchmark.java
2. https://github.com/spring-projects/spring-framework/pull/24946
Regards,
Sergey Tsypanov
> Hi Sergey,
>
> Are you taking into account that for many reads the data is not copied
> into the local buffer.
> See the comments in BufferedInputStream.read1: 277:280?
>
> How much is the slowdown, when BufferedInputStreams are chained?
>
> Thanks, Roger
>
> On 4/15/21 7:08 AM, Pavel Rappo wrote:
>
>>> On 15 Apr 2021, at 08:33, Сергей Цыпанов <sergei.tsypa...@yandex.ru> wrote:
>>>
>>> Hello,
>>>
>>> buffering with j.i.BufferedInputStream is a handy way to improve
>>> performance of IO operations.
>>> However in many cases buffering is redundant. Consider this code snippet
>>> from Spring Framework:
>>>
>>> static ClassReader getClassReader(Resource rsc) throws Exception {
>>> try (var is = new BufferedInputStream(rsc.getInputStream())) {
>>> return new ClassReader(is);
>>> }
>>> }
>>>
>>> Interface Resource has lots of implementations some of them return buffered
>>> InputStream,
>>> others don't, but all of them get wrapped with BufferedInputStream.
>>>
>>> Apart from obvious misuses like BufferedInputStream cascade such wrapping
>>> is not necessary,
>>> e.g. when we read a huge file using FileInputStream.readAllBytes(),
>>> in others cases it's harmful e.g. when we read a small (comparing to the
>>> default
>>> size of buffer in BufferedInputStream) file with readAllBytes() or
>>> when we read from ByteArrayInputStream which is kind of buffered one by its
>>> nature.
>>>
>>> I think an instance of InputStream should decide itself whether it requires
>>> buffering,
>>> so I suggest to add a couple of methods into j.i.InputStream:
>>>
>>> // subclasses can override this
>>> protected boolean needsBuffer() {
>>> return true;
>>> }
>>>
>>> public InputStream buffered() {
>>> return needsBuffer() ? new BufferedInputStream(this) : this;
>>> }
>>>
>>> this allows subclasses of InputStream to override needsBuffer() to declare
>>> buffering redundancy.
>>> Let's imagine we've overridden needsBuffer() in BufferedInputStream:
>>>
>>> public class BufferedInputStream {
>>> @Override
>>> public boolean needsBuffer() {
>>> return true;
>>> }
>>> }
>>>
>>> then the code we've mentioned above should be rewritten as
>>>
>>> static ClassReader getClassReader(Resource rsc) throws Exception {
>>> try (var is = rsc.getInputStream().buffered()) {
>>> return new ClassReader(is);
>>> }
>>> }
>>>
>>> preventing cascade of BufferedInputStreams.
>>
>> When I read this part
>>
>>> There are also cases when the need for buffering depends on the way how we
>>> read from InputStream:
>>>
>>> new FileInputStream(file).buffered().readAllBytes() // buffering is
>>> redundant
>>
>> my knee-jerk reaction was that a better solution likely lies with
>> introducing a marker interface and selectively implementing it as opposed to
>> adding two new methods to the existing class and selectively overriding
>> them. Let's call this interface java.io.Buffered: Bufferred is to
>> InputStream as RandomAccess is to List.
>>
>> Just to be clear: I'm not proposing to actually do this. It's just a thought.
>>
>> -Pavel
>>
>>> var data = new DataInputStream(new FileInputStream(file).buffered())
>>> for (int i = 0; i < 1000; i++) {
>>> data.readInt(); // readInt() does 4 calls to InputStream.read() so
>>> buffering is needed
>>> }
>>>
>>> here if FileInputStream.needsBuffer() is overridden and returns false
>>> (assuming most of reads from it are bulk)
>>> then we won't have buffering for DataInputStream. To avoid this we can also
>>> add InputStream.buffered(boolean enforceBuffering) to have manual control.
>>>
>>> To sum up, my proposal is to add those methods to InputStream:
>>>
>>> protected static boolean needsBuffer() {
>>> return true;
>>> }
>>>
>>> public InputStream buffered() {
>>> return buffered(needsBuffer());
>>> }
>>>
>>> public InputStream buffered(boolean enforceBuffering) {
>>> return enforceBuffering ? new BufferedInputStream(this) : this;
>>> }
>>>
>>> What do you think?
>>>
>>> With best regards,
>>> Sergey Tsypanov
