review for 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale

[David Holmes]

On 28/01/2012 5:25 AM, Vladimir Kozlov wrote:
> I change alias to hotspot-dev since it affects everyone.

Thanks Vladimir!

Overall I find it difficult to follow the protocol that applies here - 
in particular how safepoints can and can not appear between different 
methods and the way in which needs_gc() might change value between the 
enter and exit of a critical region. Without that understanding it is 
hard to evaluate the correctness of the new approach.

---

All: copyright dates need updating.

---

src/share/vm/runtime/safepoint.hpp

Can we add an assert that the SafepointLock is held in 
increment_jni_active_count() please.

----

src/share/vm/memory/gcLocker.hpp

54   // The _jni_lock_count is keeps

Typo: "is keeps"

---

src/share/vm/memory/gcLocker.cpp

  72       _wait_begin = os::javaTimeMillis();
  135      os::javaTimeMillis() - _wait_begin,

Are you sure you want to use the non-monotonic javaTimeMillis here? Use 
of this was recently changed elsewhere in the GC code.


Cheers,
David
-----



> In GC_locker::check_active_before_gc() move wait_begin initialization
> under Print* check since it used only under such checks. Could method
> check_active_before_gc() be called from different threads concurrently?
> Does it need lock? Note that other methods have lock.
>
> Thanks,
> Vladimir
>
> Tom Rodriguez wrote:
>> http://cr.openjdk.java.net/~never/7129164
>> 200 lines changed: 126 ins; 33 del; 41 mod; 3958 unchg
>>
>> 7129164: JNI Get/ReleasePrimitiveArrayCritical doesn't scale
>> Summary:
>> Reviewed-by:
>>
>> The machinery for GC_locker which supports GetPrimitiveArrayCritical
>> maintains a count of the number of threads that currently have raw
>> pointers exposed. This is used to allow existing critical sections to
>> drain before creating new ones so that a GC can occur. Currently
>> these counts are updated using atomic all the time, even if a GC isn't
>> being requested. This creates scalability problem when a lot of
>> threads are hammering atomic operations on the jni_lock_count. The
>> count only needs to be valid when checking if a critical section is
>> currently active and when draining the existing sections. The fix is
>> to make the count be computed as part of the safepointing machinery
>> and to only decrement the counters when _needs_gc is true. In debug
>> mode the old count is maintained and validated against the lazily
>> computed count.
>>
>> On a microbenchmark that stresses GetPrimitiveArrayCritical with many
>> threads and relatively short critical section it can more than double
>> the throughput. This also slightly speeds up the normal
>> GetPrimtiveArrayCritical calls. Mostly it's not easily measurable
>> with larger scale programs.
>>
>> Tested with microbenchmark that stresses GetPrimtiveArrayCritical and
>> the crypto benchmarks of specjvm2008 on x86 and sparc. I also ran the
>> java.io regression tests from the JDK.
>>