RFR (M) Close alignment gaps in InstanceKlass

[Dean Long]

OK, thanks, looks good!

dl

On 4/22/20 7:32 PM, coleen.phillimore at oracle.com wrote:
>
>
> On 4/22/20 9:00 PM, Dean Long wrote:
>> It looks like calling the JVMCI getSourceFileName() on an array would 
>> have accessed random memory because it was expecting an 
>> InstanceKlass.  Instead of returning null we might want to throw an 
>> exception like in HotSpotResolvedPrimitiveType.
> It was never called, except when I tried to call it on an array in the 
> test.  It caused an assert in the hotspot code. How about this? 
> Something else in that file throws JVMCIError with a similar message.
>
>     public String getSourceFileName() {
>         if (isArray()) {
>             throw new JVMCIError("Cannot call getSourceFileName() on 
> an array klass type: %s", this);
>         }
>         return getConstantPool().getSourceFileName();
>     }
>
>>
>> dl
>>
>> On 4/22/20 5:39 PM, Dean Long wrote:
>>> Can you compare the result to some string, like "Object.java"?  That 
>>> seems to be what HotSpotResolvedObjectTypeTest.java is doing.
>>> Also, did getSourceFileName() return null for arrays before your 
>>> change?
>
> Fixed:
>     public void getSourceFileNameTest() {
>         Class<?> c = Object.class;
>         ResolvedJavaType type = metaAccess.lookupJavaType(c);
>         assertEquals(type.getSourceFileName(), "Object.java");
>     }
>
> thanks,
> Coleen
>
>>>
>>> dl
>>>
>>> On 4/22/20 8:18 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Hi Dean, Thank you for looking at the JVMCI changes and the 
>>>> suggestion to add the test.  I did this and found a bug. The new 
>>>> test is quite limited because there's no good test to see if a 
>>>> source file name can assertNotNull(type.getSourceFileName()), so I 
>>>> couldn't iterate through the list of loaded classes like the other 
>>>> tests in that file.
>>>>
>>>> http://cr.openjdk.java.net/~coleenp/2020/8238048.02.incr/webrev/index.html 
>>>>
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>
>>>> On 4/21/20 9:51 PM, Dean Long wrote:
>>>>> Hi Coleen.  The JVMCI changes look OK. It looks like there is a 
>>>>> Graal unittest that covers getSourceFileName, but those tests 
>>>>> don't always get run. If it's not too much trouble, could you look 
>>>>> into enabling getSourceFileName() testing in
>>>>>
>>>>> test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java 
>>>>>
>>>>>
>>>>> ?  It's currently on the "untested" list.
>>>>>
>>>>> thanks,
>>>>>
>>>>> dl
>>>>>
>>>>> On 4/21/20 1:12 PM, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: moved fields around and some constant fields into 
>>>>>> ConstantPool
>>>>>>
>>>>>> This is a simple change except that I moved some constant fields 
>>>>>> from InstanceKlass into the constant pool so they can be shared 
>>>>>> read-only in the CDS archive.  There are associated repercussions 
>>>>>> in SA and JVMCI, so please look at these changes. Also moved 
>>>>>> similarly sized fields together in the class so there's less 
>>>>>> likelihood of introducing gaps in future InstanceKlass changes.
>>>>>>
>>>>>> InstanceKlass is reduced from 544 to 520 bytes in a simple Hello 
>>>>>> World class.
>>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8238048
>>>>>>
>>>>>> Tested with tier1-6.
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>