harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vijay Menon <...@google.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Mon, 09 Nov 2009 17:05:09 GMT
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.ellison@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.ellison@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;
> >>             }
> >>
> >>
> >
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message