[9, 8u40] RFR (M): 8057020: LambdaForm caches should support eviction

[Peter Levart]

(Sorry for a re-send, forgot to include core-libs-dev)...

Hi Vladimir,

First, just a nit. I think that in LambdaFormEditor:

  289     private LambdaForm putInCache(Transform key, LambdaForm form) {
  290         key = key.withResult(form);
  291         for (int pass = 0; ; pass++) {
  292             Object c = lambdaForm.transformCache;
  293             if (c instanceof ConcurrentHashMap) {
  294                 @SuppressWarnings("unchecked")
  295                 ConcurrentHashMap<Transform,Transform> m = (ConcurrentHashMap<Transform,Transform>) c;
  296                 Transform k = m.putIfAbsent(key, key);
  297                 if (k == null) return form;
  298                 LambdaForm result = k.get();
  299                 if (result != null) {
  300                     return result;
  301                 } else {
  302                     if (m.replace(key, k, key)) {
  303                         return form;
  304                     } else {
  305                         continue;
  306                     }
  307                 }
  308             }
  309             assert(pass == 0);
  310             synchronized (lambdaForm) {
  311                 c = lambdaForm.transformCache;
  312                 if (c instanceof ConcurrentHashMap)
  313                     continue;

...

  372                     lambdaForm.transformCache = c = m;
                                                      ^^^ put assignment to 'c' back in
  373                     // The second iteration will update for this query, concurrently.
  374                     continue;


...you could move the assignment to 'c' in line 292 out of for loop and 
put it back in line 372, since once 'c' is instance of CHM, 
lambdaForm.transformCache never changes again and if 'c' is not CHM yet, 
it is re-assigned in lines 311 and 372 before next loop.

Am I right?

Now what scares me (might be that I don't have an intimacy with 
LambdaForm class like you do). There is a situation where you publish 
LambdaForm instances via data race.

One form of LambdaForm.transformCache is an array of Transform objects 
(the other two forms are not problematic). Transform class has all 
fields final except the 'referent' field of SoftReference, which holds a 
LambdaForm instance. In the following line:

377                 ta[idx] = key;


...you publish Transform object to an element of array with relaxed 
write, and in the following lines:

  271         } else {
  272             Transform[] ta = (Transform[])c;
  273             for (int i = 0; i < ta.length; i++) {
  274                 Transform t = ta[i];
  275                 if (t == null)  break;
  276                 if (t.equals(key)) { k = t; break; }
  277             }
  278         }
  279         assert(k == null || key.equals(k));
  280         return (k != null) ? k.get() : null;


...you obtain the element of the array with no synchronization and a 
relaxed read and might return a non-null referent (the LambdaForm) which 
is then returned as an interned instance.

So can LambdaForm instances be published via data races without fear 
that they would appear half-initialized?

That's what I didn't know when I used a lazySet coupled with volatile 
get to access array elements in my version:

http://cr.openjdk.java.net/~plevart/misc/LambdaFormEditor.WeakCache/webrev.01/


Regards, Peter

On 12/03/2014 12:45 PM, Vladimir Ivanov wrote:
> Aleksey, thanks for the review.
>
> I haven't tried -XX:SoftRefLRUPolicyMSPerMB=0, but I did extensive 
> testing on Octane/Nashorn with multiple low -Xmx levels + frequent 
> Full GCs (8060147 [1] was the result of those experiments) and stress 
> tested cache eviction with jdk/java/lang/invoke/LFCache tests in long 
> running mode.
>
> Best regards,
> Vladimir Ivanov
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8060147
>
> On 12/3/14, 3:11 PM, Aleksey Shipilev wrote:
>> On 12/01/2014 07:58 PM, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~vlivanov/8057020/webrev.00/
>>> https://bugs.openjdk.java.net/browse/JDK-8057020
>>
>> Looks okay, although the cache management logic gives me a headache
>> after the vacation. I thought I spotted a few bugs, but those were only
>> false positives.
>>
>>> The fix is to use SoftReferences to keep LambdaForms alive as long as
>>> possible, but avoid throwing OOME until the caches are evicted. I
>>> experimented with WeakReferences, but it doesn't hold LambdaForms for
>>> long enough: LambdaForm cache hit rate degrades significantly and it
>>> negatively affects application startup and warmup, since every
>>> instantiated LambdaForm is precompiled to bytecode before usage.
>>>
>>> Testing: jdk/java/lang/invoke/LFCache in stress mode + jck
>>> (api/java_lang/invoke), jdk/java/lang/invoke, jdk/java/util/streams, 
>>> octane
>>
>> SoftReferences are tricky in the way they can get suddenly drop the
>> referent, and normal testing would not catch it (e.g. the normal
>> operation would reclaim softrefs under your feet almost never). Does
>> this code survive with -XX:SoftRefLRUPolicyMSPerMB=0?
>>
>> Thanks,
>> -Aleksey.
>>
>>
> _______________________________________________
> mlvm-dev mailing list
> mlvm-dev at openjdk.java.net
> http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/mlvm-dev/attachments/20141206/02a15ebe/attachment.html>