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