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 Thu, 12 Nov 2009 10:55:43 GMT
On 09/Nov/2009 17:05, Vijay Menon wrote:
> 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.

Really?  Even if they are not synchronized/volatile writes?  We account
for the fact that the writes may be unsafe.

Regards,
Tim


> 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
View raw message