harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vijay Menon <...@google.com>
Subject Re: [classlib] Fixing an unsafe reference in InputStream
Date Thu, 12 Nov 2009 19:20:45 GMT
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 <alexei.fedotov@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.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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message