Perhaps a dumb question - but is there a really a performance issue with
always using a stack-local localBuf and removing the shared static? The
code is simpler, and memory allocation is usually a fast operation.
The code below does look semantically safe, but there is a potential
performance issue if multiple threads are executing this code concurrently
and writing to the same block of memory. I.e., the underlying cache lines
will ping-pong between cores.
Cheers,
Vijay
2009/11/9 Tim Ellison <t.p.elli...@gmail.com>
> On 05/Nov/2009 20:43, Alexei Fedotov wrote:
> > I enjoyed the simple, but neat optimization. Let me question if the
> > enlarged buffer should be stored at skipBuf.
> >
> > 1. Escaping reference always causes optimization problems. A
> > reasonable JIT can drop a local array allocation if it is possible to
> > de-virtualize the call and see that the array is never read.
>
> True, perhaps we should use a stack allocated localBuf if the skip size
> 'n' is small, e.g.
>
> Index: src/main/java/java/io/InputStream.java
> ===================================================================
> --- src/main/java/java/io/InputStream.java (revision 833452)
> +++ src/main/java/java/io/InputStream.java (working copy)
> @@ -211,14 +211,19 @@
> }
> long skipped = 0;
> int toRead = n < 4096 ? (int) n : 4096;
> + byte[] localBuf;
> + if (n < 128) {
> + localBuf = new byte[(int)n];
> + } else {
> // We are unsynchronized, so take a local copy of the skipBuf
> at some
> // point in time.
> - byte[] localBuf = skipBuf;
> + localBuf = skipBuf;
> if (localBuf == null || localBuf.length < toRead) {
> // May be lazily written back to the static. No matter if it
> // overwrites somebody else's store.
> skipBuf = localBuf = new byte[toRead];
> }
> + }
> while (skipped < n) {
> int read = read(localBuf, 0, toRead);
> if (read == -1) {
>
>
> However, I've no evidence that small skips are prevalent and that the
> optimization would be useful.
>
> > 2. Those ones who skip a stream to the end will have static skipBuf
> > always of 4K bytes. Why not to allocate 4K on the first allocation,
> > and save a localBuf.length access?
>
> well, I guess the obvious answer is because we want to reduce the
> retained memory in cases where the skips are small. Again, not sure
> what the usage really is that justifies either approach.
>
> Regards,
> Tim
>
> > On Thu, Oct 1, 2009 at 5:19 PM, Tim Ellison <t.p.elli...@gmail.com>
> wrote:
> >> Could somebody please take a look a the patch below and see if they
> >> believe my explanation for why it fixes a findbugs problem in
> InputStream?
> >>
> >> The problem: We have a static, skipBuf, that we only use to read junk
> >> data from the stream when skipping a number of bytes. The static is
> >> lazily initialized. It may be null, or too small for the job. The
> >> problem is that another thread could also see null (or a small buffer)
> >> and then set the buffer too small after the calling thread sets it to
> >> /their/ required size.
> >>
> >> The solution: If we take a local copy, localBuf, as a snapshot of
> >> skipBuf before testing it, then even if the skipBuf value is slammed by
> >> another thread we have our local copy. We can also blindly overwrite
> >> any value stored by other threads since they will be doing the same
> thing.
> >>
> >> So the update of the static is still unsafe, but the usage of it is ok
> >> -- and this is better than making it volatile.
> >>
> >> Agree?
> >>
> >> Tim
> >>
> >>
> >> Index: InputStream.java
> >> ===================================================================
> >> --- InputStream.java (revision 820251)
> >> +++ InputStream.java (working copy)
> >> @@ -211,11 +211,16 @@
> >> }
> >> long skipped = 0;
> >> int toRead = n < 4096 ? (int) n : 4096;
> >> - if (skipBuf == null || skipBuf.length < toRead) {
> >> - skipBuf = new byte[toRead];
> >> + // We are unsynchronized, so take a local copy of the skipBuf
> >> at some
> >> + // point in time.
> >> + byte[] localBuf = skipBuf;
> >> + if (localBuf == null || localBuf.length < toRead) {
> >> + // May be lazily written back to the static. No matter if
> it
> >> + // overwrites somebody else's store.
> >> + skipBuf = localBuf = new byte[toRead];
> >> }
> >> while (skipped < n) {
> >> - int read = read(skipBuf, 0, toRead);
> >> + int read = read(localBuf, 0, toRead);
> >> if (read == -1) {
> >> return skipped;
> >> }
> >>
> >>
> >
> >
> >
>
