RFR: 8031639: make dependency management (mostly) ci independent (original) (raw)

Roland Westrelin roland.westrelin at oracle.com
Mon Jan 20 10:05:59 PST 2014

Doug,

Comments inlined below

Thanks for the review. Responses inline below:

On Jan 20, 2014, at 5:13 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

Hi Doug,

webrev: http://cr.openjdk.java.net/~dnsimon/JDK-8031639/ More informative assert messages than “oops” would be appreciated. Sorry, I’ll change the message to something more descriptive. dependencies.hpp: comment needs to be fixed: 249 GrowableArray* depseen; // (seen[h->ident] & (1<<dept)) How would you like it changed (I don’t think I modified this from its original). Are you suggesting the comment needs to reflect its usage in both the original and the #ifdef GRAAL world?

I think h->ident is a reference to _ident from ciBaseObject which you don’t use anymore. A comment that states that you’re using some unique identifier as computed by DepValue would be clearer.

dependencies.cpp:

75 if (isjavaprimitive(elemt)) return; // Ex: int[][] align comment with // Ex: String[][] Will do. 121 void Dependencies::assertcallsitetargetvalue(jobject callsite, jobject methodhandle) { 122 Klass* ctxk = JNIHandles::resolve(callsite)->klass(); 123 checkctxk(ctxk); 124 assertcommon2(callsitetargetvalue, DepValue(ooprecorder, callsite), DepValue(ooprecorder, methodhandle)); 125 } Don’t you need a VMENTRY? Actually, the signature and implementation in this patch is a little of our date with respect to the current Graal code base. It’s now: void Dependencies::assertcallsitetargetvalue(oop callsite, oop methodhandle) { assertcommon2(callsitetargetvalue, DepValue(ooprecorder, JNIHandles::makelocal(callsite)), DepValue(ooprecorder, JNIHandles::makelocal(methodhandle))); } This is only called from a point within a VMENTRY so I don’t think it needs another VMENTRY marker, right? Is there someway to assert whether code is within a VMENTRY?

ASSERT_IN_VM is what you would use, I think.

I don’t see the VM_ENTRY when it’s called from GraphBuilder::access_field() for instance.

Have you verified that compilation time is not affected? No. Can you please suggest a reliable way to do this? I’m assuming something like CompileTheWorld?

I was not suggesting a thorough experiment. Running your favorite benchmark with -XX:+CITime and checking nothing surprising happens is what I had in mind.

I don’t really feel comfortable with this change. I wonder if it wouldn’t be simpler to have a base class for Dependencies, then one subclass for Dependencies that work on ci objects and one subclass that work directly on objects and Metadata. Then either accept code duplication or maybe use a parallel hierarchy for DepValue. If the compilation time is not impacted, wouldn’t it be better to avoid code duplication altogether?

To me, this change doesn’t fit well with existing abstractions. I don’t think ciObject::constant_encoding() exists so that we can call JNIHandles::resolve() on the result and get a naked oop. All metadata references are still embedded in ciMetadata and even though it’s safe to grab a direct pointer to a Metadata from the compilers with the current implementation of the meta data, we don’t do it. That’s why I’m not comfortable with this change. I don’t have a nice solution to suggest either.

Roland.

I’ll post another webrev after waiting a bit for further feedback. -Doug

More information about the hotspot-compiler-dev mailing list