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

Doug Simon doug.simon at oracle.com
Mon Jan 20 13:12:15 PST 2014

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

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))); }

Ignore my above “correction” - it only applies to the code we have today because we maintain the Graal way of doing things separately from the ci* way of doing things. It’s exactly this duplication this changeset aims to remove.

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? ASSERTINVM is what you would use, I think. I don’t see the VMENTRY when it’s called from GraphBuilder::accessfield() for instance.

Stepping back a moment, what is the requirement for VM_ENTRY here?

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.

It does’t seem to make any real difference. In fact, the "Code Installation” time after applying the changes decreased by about 4% although it’s hard to say what the error bars are for this measurement. See the details of my mini-experiement at the end of this message.

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::constantencoding() exists so that we can call JNIHandles::resolve() on the result and get a naked oop.

This change never exposes a naked oop - they all remain in jobjects.

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.

I can understand that extracting the raw pointers from ciMetadata needs to be done carefully. However, the only difference this change makes is that it extracts the raw values earlier (i.e. as dependencies are recorded) instead of when they are serialized (in Dependencies::encode_content_bytes()).

-Doug

==== Testing compilation time impact with -XX:+CITime and the DaCapo tradebeans benchmark ====

Original:

$ java -server -XX:+CITime -jar lib/dacapo-9.12-bach.jar tradebeans -n 10 Using scaled threading model. 8 processors detected, 8 threads used to drive the workload, in a possible range of [1,512] Booting Geronimo Kernel (in Java 1.8.0-ea)… … Accumulated compiler times (for compiled methods only)

Total compilation time : 24.882 s Standard compilation : 23.599 s, Average : 0.004 On stack replacement : 1.283 s, Average : 0.019 Detailed C1 Timings Setup time: 0.000 s ( 0.0%) Build IR: 1.295 s (41.8%) Optimize: 0.046 s ( 1.5%) RCE: 0.010 s ( 0.3%) Emit LIR: 0.798 s (25.8%) LIR Gen: 0.196 s ( 6.3%) Linear Scan: 0.595 s (19.2%) LIR Schedule: 0.000 s ( 0.0%) Code Emission: 0.255 s ( 8.2%) Code Installation: 0.748 s (24.2%) Instruction Nodes: 435649 nodes

Total compiled methods : 6528 methods Standard compilation : 6461 methods On stack replacement : 67 methods Total compiled bytecodes : 1305736 bytes Standard compilation : 1256020 bytes On stack replacement : 49716 bytes Average compilation speed: 52475 bytes/s

nmethod code size : 12021920 bytes nmethod total size : 22180800 bytes

Modified:

$ java -server -XX:+CITime -jar lib/dacapo-9.12-bach.jar tradebeans -n 10 Using scaled threading model. 8 processors detected, 8 threads used to drive the workload, in a possible range of [1,512] Booting Geronimo Kernel (in Java 1.8.0-ea)… … Accumulated compiler times (for compiled methods only)

Total compilation time : 23.023 s Standard compilation : 21.679 s, Average : 0.003 On stack replacement : 1.344 s, Average : 0.024 Detailed C1 Timings Setup time: 0.000 s ( 0.0%) Build IR: 1.275 s (42.9%) Optimize: 0.043 s ( 1.4%) RCE: 0.009 s ( 0.3%) Emit LIR: 0.863 s (29.1%) LIR Gen: 0.278 s ( 9.4%) Linear Scan: 0.579 s (19.5%) LIR Schedule: 0.000 s ( 0.0%) Code Emission: 0.244 s ( 8.2%) Code Installation: 0.589 s (19.8%) Instruction Nodes: 439540 nodes

Total compiled methods : 6437 methods Standard compilation : 6382 methods On stack replacement : 55 methods Total compiled bytecodes : 1289495 bytes Standard compilation : 1242674 bytes On stack replacement : 46821 bytes Average compilation speed: 56009 bytes/s

nmethod code size : 12182368 bytes nmethod total size : 22058816 bytes

More information about the hotspot-compiler-dev mailing list