harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Ellison <t.p.elli...@gmail.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Mon, 09 Nov 2009 11:43:38 GMT
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
View raw message