RFR: 8031639: make dependency management (mostly) ci independent
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<int>* _dep_seen; // (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 (is_java_primitive(elemt)) return; // Ex: int[][]
>>> align comment with // Ex: String[][]
>>
>> Will do.
>>
>>> 121 void Dependencies::assert_call_site_target_value(jobject call_site, jobject method_handle) {
>>> 122 Klass* ctxk = JNIHandles::resolve(call_site)->klass();
>>> 123 check_ctxk(ctxk);
>>> 124 assert_common_2(call_site_target_value, DepValue(_oop_recorder, call_site), DepValue(_oop_recorder, method_handle));
>>> 125 }
>>>
>>> Don’t you need a VM_ENTRY?
>>
>> 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::assert_call_site_target_value(oop call_site, oop method_handle) {
>> assert_common_2(call_site_target_value, DepValue(_oop_recorder, JNIHandles::make_local(call_site)), DepValue(_oop_recorder, JNIHandles::make_local(method_handle)));
>> }
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 VM_ENTRY so I don’t think it needs another VM_ENTRY marker, right? Is there someway to assert whether code is within a VM_ENTRY?
>
> 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.
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::constant_encoding() 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