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, 12 Nov 2009 11:07:44 GMT
Tim,
AFAIU, this is a second level effect. By themselves unsynchronized
writes are quick and parallel. But.

The processor cache line which corresponds to the common buffer is
always invalidated. This means that the common bus is constantly
stressed with synchronization. My friend Alexei Shipilev told me two
years ago that some real loads he investigated stressed the bus to the
full capacity, so that loads could benefit from freeing the bus.



On Thu, Nov 12, 2009 at 1:55 PM, Tim Ellison <t.p.ellison@gmail.com> wrote:
> 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;
>>>>>             }
>>>>>
>>>>>
>>>>
>>>>
>>
>



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