You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Tim Ellison <t....@gmail.com> on 2009/10/01 16:19:53 UTC
[classlib] Fixing an unsafe reference in InputStream
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;
}
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Alexei Fedotov <al...@gmail.com>.
I support reluctance in fixing performance. AFAIK, no performance
change should be done without demonstrating incerase on real loads.
2009/11/13 Tim Ellison <t....@gmail.com>:
> On 12/Nov/2009 19:20, Vijay Menon wrote:
>> Right, writes - even unsynchronized ones - to the same cache line are
>> problematic. Here's a good quick read on this:
>> http://en.wikipedia.org/wiki/MESI_protocol. Recent Intel architectures
>> adapt this protocol. Here's the interesting bit:
>>
>> A write may only be performed if the cache line is in the Modified or
>> Exclusive state. If it is in the Shared state, all other cached copies must
>> be invalidated first. This is typically done by a broadcast operation known
>> as *Read For Ownership (RFO)*.
>>
>>
>> If two cores continually write to the same cache line, they'll repeatedly
>> invalidate each other.
>
> Thanks for the pointers.
>
> Given that the current code has been there since time immemorial and is
> not broken, I'm inclined to leave it for now and pick this thread up
> again after M12. That'll give us time to see what the difference is
> between the local and static versions (though if I were a betting person
> I'd say skip is used infrequently enough that nobody's app will notice).
>
> Regards,
> Tim
>
--
With best regards / с наилучшими пожеланиями,
Alexei Fedotov / Алексей Федотов,
http://www.telecom-express.ru/
http://harmony.apache.org/
http://www.expressaas.com/
http://openmeetings.googlecode.com/
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Tim Ellison <t....@gmail.com>.
On 12/Nov/2009 19:20, Vijay Menon wrote:
> Right, writes - even unsynchronized ones - to the same cache line are
> problematic. Here's a good quick read on this:
> http://en.wikipedia.org/wiki/MESI_protocol. Recent Intel architectures
> adapt this protocol. Here's the interesting bit:
>
> A write may only be performed if the cache line is in the Modified or
> Exclusive state. If it is in the Shared state, all other cached copies must
> be invalidated first. This is typically done by a broadcast operation known
> as *Read For Ownership (RFO)*.
>
>
> If two cores continually write to the same cache line, they'll repeatedly
> invalidate each other.
Thanks for the pointers.
Given that the current code has been there since time immemorial and is
not broken, I'm inclined to leave it for now and pick this thread up
again after M12. That'll give us time to see what the difference is
between the local and static versions (though if I were a betting person
I'd say skip is used infrequently enough that nobody's app will notice).
Regards,
Tim
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Vijay Menon <vs...@google.com>.
Right, writes - even unsynchronized ones - to the same cache line are
problematic. Here's a good quick read on this:
http://en.wikipedia.org/wiki/MESI_protocol. Recent Intel architectures
adapt this protocol. Here's the interesting bit:
A write may only be performed if the cache line is in the Modified or
Exclusive state. If it is in the Shared state, all other cached copies must
be invalidated first. This is typically done by a broadcast operation known
as *Read For Ownership (RFO)*.
If two cores continually write to the same cache line, they'll repeatedly
invalidate each other.
Cheers,
Vijay
2009/11/12 Alexei Fedotov <al...@gmail.com>
> 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....@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....@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....@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/
>
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Alexei Fedotov <al...@gmail.com>.
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....@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....@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....@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/
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Tim Ellison <t....@gmail.com>.
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....@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....@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;
>>>> }
>>>>
>>>>
>>>
>>>
>
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Vijay Menon <vs...@google.com>.
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.
Cheers,
Vijay
2009/11/9 Tim Ellison <t....@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....@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;
> >> }
> >>
> >>
> >
> >
> >
>
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Tim Ellison <t....@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....@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;
>> }
>>
>>
>
>
>
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Alexei Fedotov <al...@gmail.com>.
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....@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/
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Tim Ellison <t....@gmail.com>.
Thanks guys, fixed at r820776.
Regards,
Tim
On 01/Oct/2009 18:11, Oliver Deakin wrote:
> Tim Ellison 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?
>>
>
> +1 from me.
>
> Regards,
> Oliver
>
>> 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;
>> }
>>
>>
>>
>
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Oliver Deakin <ol...@googlemail.com>.
Tim Ellison 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?
>
+1 from me.
Regards,
Oliver
> 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;
> }
>
>
>
--
Oliver Deakin
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Re: [classlib] Fixing an unsafe reference in InputStream
Posted by Jesse Wilson <je...@google.com>.
On Thu, Oct 1, 2009 at 7:19 AM, Tim Ellison <t....@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?
>
Looks good to me.
Yes, this should fix the concurrency problem.