Re: RFR (S) CR 7177472: JSR292: MethodType interning penalizes scalability

[Peter Levart]

On 06/08/2013 10:25 PM, Peter Levart wrote:

On 06/08/2013 10:17 PM, Peter Levart wrote:

On 06/08/2013 12:21 PM, Remi Forax wrote:
On 06/07/2013 12:01 PM, Aleksey Shipilev wrote:
(posting this to hotspot-dev@ and cc-ing core-libs-dev@, as 
Christian T.
suggested offline)

Hi guys,

Hi Aleksey,


The fix for scalability problem is here:
http://cr.openjdk.java.net/~shade/7177472/webrev.00/

in add, the do/while does something a little weird, if putIfAbsent 
returns null,
interned is equals to elem, there is no need to do a e.get() in that 
case,
I would prefer a code like this:

            T interned;
            do {
                expungeStaleElements();
                WeakEntry<T> e = new WeakEntry<>(elem, stale);
                WeakEntry<T> exist = map.putIfAbsent(e, e);
                interned = (exist == null)? elem: exist.get();
            } while (interned == null);
            return interned;

Hi Remi, Aleksey,

In case the loop retries, there's no need to construct another 
WeakEntry...

            T interned;
             WeakEntry<T> e = new WeakEntry<>(elem, stale);
            do {
                expungeStaleElements();
                WeakEntry<T> exist = map.putIfAbsent(e, e);
                interned = (exist == null)? elem: exist.get();
            } while (interned == null);
            return interned;

Regards, Peter

Aleksey,

Also, in a retry-loop you might be spin-waiting for the 
ReferenceHandler thread to transfer the cleared WeakEntry to the 
ReferenceQueue. You might want to do a map.replace(e, exist, e) in 
case exist != null and exist.get() == null to make it independent of 
ReferenceHandler thread's processing. In this case only a single 
out-of-loop call to expungeStaleElements() would be enough.

Scrap that. when exist.get() == null, exist.equals(e) == false, so 
putIfAbsent should succeed.


Peter



The cast to WeakEntry in expungeStaleElements is not needed.

In WeakEntry.equals, the null check is not needed because null 
instanceof WeakEntry returns false,
and you don't need to cast obj.get() to a WeakEntry<T> given you 
only to call equals on entry.get().
So the code should be:


           public boolean equals(Object obj) {
             if (!(obj instanceof WeakEntry)) {
               return false;
             }
               Object that = ((WeakEntry<?>)obj).get();
             Object mine = get();
             return (that == null || mine == null)? this == obj: 
mine.equals(that);
         }


Otherwise, it looks good.

Rémi


Testing:
   - Linux x86_64 builds OK
   - Linux x86_64 java/lang/invoke/ jtreg passes OK

Since this is a proof-of-concept patch at this point, I kept the 
testing
minimal. Let me know what other testing you think is needed before the
integration.

Thanks,
Aleksey.

*** Rationale and details for the issue follow: ***

This issue was in limbo for an year. The performance issue with
MethodType.methodType factory leads to abysmal scalability on the
targeted tests. While this can arguably demonstrated on larger
workloads, the targeted microbenchmarks are showcasing it nicely. This
issue was blocking the JSR292 API performance testing, because on 
large
enough test server, you will inevitably be bitten by this.

If you run the JMH-enabled [1] microbenchmark:
http://openjdk.java.net/projects/code-tools/jmh/
http://cr.openjdk.java.net/~shade/7177472/jsr292bench.tar.gz

...on the current jdk8/jdk8, 2x2 i5 2.0 Ghz, Linux x86_64, you will 
see
the scalability is going down. ("testCached" makes sure the 
reference to
MethodType is always strongly reachable, "testNew" makes sure the
reference is mostly non-reachable, prompting cache re-fill, "testOOB"
does not make any precautions about that, and also does not suffer 
from
the static overheads entailed by the reference management in first two
tests).

1 thread:
MethodTypeBench.testCached     78 +-  1 nsec/op
MethodTypeBench.testNew       120 +-  1 nsec/op
MethodTypeBench.testOOB        41 +-  1 nsec/op

4 threads:
MethodTypeBench.testCached    495 +- 40 nsec/op
MethodTypeBench.testNew       611 +- 37 nsec/op
MethodTypeBench.testOOB       377 +- 10 nsec/op

In fact, on larger machines it may be as bad as tens/hundreds of
microseconds to look up the MethodType. With the patch applied, on the
same 2x2 machine:

1 thread:
MethodTypeBench.testCached     92 +-  1 nsec/op
MethodTypeBench.testNew       101 +-  1 nsec/op
MethodTypeBench.testOOB        49 +-  1 nsec/op

4 threads:
MethodTypeBench.testCached    129 +-  1 nsec/op
MethodTypeBench.testNew       142 +- 10 nsec/op
MethodTypeBench.testOOB        89 +-  3 nsec/op


...which means the scalability is back: the average time to look up 
the
MethodType is not growing spectacularly when the thread count go up.
Again, the effect is astonishing on large machines.

Notice that we took some hit in single-threaded performance, and 
that is
due to two reasons: a) WeakEntry allocation in get(); b) CHM. With the
upcoming CHMv8, this starts to look better:

1 thread:
MethodTypeBench.testCached     87 +-  1 nsec/op
MethodTypeBench.testNew        95 +-  1 nsec/op
MethodTypeBench.testOOB        52 +-  1 nsec/op

4 threads:
MethodTypeBench.testCached    121 +-  2 nsec/op
MethodTypeBench.testNew       141 +-  1 nsec/op
MethodTypeBench.testOOB        93 +-  5 nsec/op

...which seems to indicate this change is future-proof.