harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexei Fedotov <alexei.fedo...@gmail.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Thu, 05 Nov 2009 20:43:00 GMT
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.
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?



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;
>             }
>
>



-- 
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/

Mime
View raw message