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.elli...@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.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;
>>>> }
>>>>
>>>>
>>>
>>>
>
