RFC: Code cache scan should use RESOLVE?

[Roman Kennke]

Ok.

I reviewed some code. The most critical one seems to be the code
patching routine in c1_Runtime1.cpp. It first acquires the
Patching_lock, and then goes ahead and patches in some oops (java
mirrors and appendices for invokevirtual). It then gives up the
Patching_lock, acquires the CodeCache_lock and calls register_nmethod()
of the heap. This is where we initially update code cache roots (by
employing a write-barrier).

This leaves a window where the oop in code cache may point to from-
space, and this is most likely where our assert happened. This is
supported by the fact that in all cases where we've seen the assert, it
was running with -XX:TieredStopAtLevel=1.

However, I believe it should be safe to use RESOLVE there. While this
is slightly racy, it should not affect our correctness. It either sees
NULL, in which case there's nothing to mark, or it sees a from-space
ref, in which case it resolves to a to-space ref and marks that. Or it
sees a to-space ref in which case it should be happy.

The patching code does give me itches (mostly because I don't
understand it fully):
 - what's in the oop location before patching? I do hope it is NULL or
some dummy object
- can execution ever get to a patched-but-not-yet-updated oop? This
would violate our assumption that compiled constants are to-space
always.

However, if any such thing would be the case, we'd fail with or without
concurrent code root scanning. I assume it's fine for now until I have
indication that it's not. ;-)

Which means to say: would you please re-enable your change?

Also, for JDK8u, I suggest disabling concurrent code root scanning by
default, and making it experimental. Don't want to get nasty surprises
there...

Roman


Am Montag, den 13.02.2017, 11:15 +0100 schrieb Aleksey Shipilev:
> Yeah.
> 
> Disabled:
>  http://hg.openjdk.java.net/shenandoah/jdk9/hotspot/rev/71679a736a4d
> 
> -Aleksey
> 
> On 02/13/2017 11:10 AM, Roman Kennke wrote:
> > In fact, I thought about it a little more. Oops in the code cache
> > should never point to from-space. When installing a new nmethod, we
> > ensure all oops are pointing to to-space. Then, during final-mark
> > pause, we evacuate + update all code cache roots. If we hit an
> > assert
> > there, something else went very wrong.
> > 
> > Could you please revert that change? I will investigate the issue.
> > 
> > Roman
> > 
> > 
> > Am Montag, den 13.02.2017, 09:50 +0100 schrieb Aleksey Shipilev:
> > > Hi,
> > > 
> > > Looking through nightly test failures, this caught my eye:
> > > 
> > > #  Internal Error
> > > (/opt/jenkins/workspace/jdk9-shenandoah-
> > > fastdebug/hotspot/src/share/vm/gc/shenandoah/shenandoahConcurrent
> > > Mark
> > > .inline.hpp:247),
> > > pid=16927, tid=16935
> > > #  assert(oopDesc::unsafe_equals(obj,
> > > ShenandoahBarrierSet::resolve_oop_static(obj))) failed: need to-
> > > space 
> > > object here
> > > 
> > > V  [libjvm.so+0xa6653a]  report_vm_error(char const*, int, char
> > > const*, char
> > > const*, ...)+0xea
> > > V  [libjvm.so+0x143f2d5]  ShenandoahMarkRefsClosure::do_oop(oop*)
> > > +0x1
> > > 15
> > > V  [libjvm.so+0x11f1862]  nmethod::oops_do(OopClosure*,
> > > bool)+0x542
> > > V  [libjvm.so+0xd74a89]  CodeBlobToOopClosure::do_code_blob(CodeB
> > > lob*
> > > )+0x29
> > > V  [libjvm.so+0x985523]  CodeCache::blobs_do(CodeBlobClosure*)+0x
> > > 163
> > > V  [libjvm.so+0x148e03f]  SCMConcurrentMarkingTask::work(unsigned
> > > int)+0x1df
> > > V  [libjvm.so+0x16d9225]  GangWorker::loop()+0xc5
> > > V  [libjvm.so+0x1252152]  thread_native_entry(Thread*)+0x112
> > > 
> > > It seems to me that with ShenandoahConcurrentCodeRoots enabled
> > > recently, we are
> > > hitting this path. And oops coming from code roots may be stale,
> > > so
> > > we need to
> > > RESOLVE them? If so, here's a blind patch:
> > >   http://cr.openjdk.java.net/~shade/shenandoah/codecache-roots-as
> > > sert
> > > /webrev.01/
> > > 
> > > I was unable to replicate the failure locally, so this is a shot
> > > in
> > > the dark.
> > > 
> > > Thanks,
> > > -Aleksey
> > > 
> 
>