RFR (S): 8214086: [TESTBUG] Fix subgraph test cases in ReplaceCriticalClasses.java

[Ioi Lam]

Hi Jiangli,

The new tests looks good.

Thanks

- Ioi


On 11/25/18 4:06 PM, Jiangli Zhou wrote:
> I realized that we need '@requires vm.cds.archived.java.heap' for the 
> archived subgraph tests. Instead of applying the restriction to all 
> test cases in ReplaceCriticalClasses.java, I moved the three 
> '-subgraph' test cases out and placed them in 
> ReplaceCriticalClassesForSubgraphs.java. 
> ReplaceCriticalClassesForSubgraphs extends from ReplaceCriticalClasses 
> and reuses the existing test logic without duplicating:
>
> http://cr.openjdk.java.net/~jiangli/8214086/webrev.02/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClassesForSubgraphs.java.html 
>
>
> Full webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.02/
>
> Thanks,
> Jiangli
>
> On 11/21/18 5:34 PM, Jiangli Zhou wrote:
>> Thanks!
>>
>> Jiangli
>>
>>
>> On 11/21/18 5:34 PM, Ioi Lam wrote:
>>> Looks good. Thanks!
>>>
>>> - Ioi
>>>
>>> On 11/21/18 4:34 PM, Jiangli Zhou wrote:
>>>> Here is the updated webrev with a new test case suggested by Ioi:
>>>>
>>>> http://cr.openjdk.java.net/~jiangli/8214086/webrev.01/test/hotspot/jtreg/runtime/SharedArchiveFile/serviceability/ReplaceCriticalClasses.java.frames.html 
>>>>
>>>>
>>>> Full webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.01/
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>> On 11/21/18 3:23 PM, Jiangli Zhou wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> On 11/21/18 3:08 PM, Ioi Lam wrote:
>>>>>> Hi Jiangli,
>>>>>>
>>>>>> For this test
>>>>>>
>>>>>> final String subgraphInit = "initialize_from_archived_subgraph " 
>>>>>> + subgraphKlass;
>>>>>> ...
>>>>>> if (checkSubgraph) {
>>>>>>      out.shouldNotContain(subgraphInit);
>>>>>> }
>>>>>>
>>>>>> I think you should also test for the reverse condition -- if 
>>>>>> java/lang/Long has not been redefined, you should see the 
>>>>>> subgraphInit in the output.
>>>>>>
>>>>>> The reason for this is shouldNotContain() is very lenient. If you 
>>>>>> made a mistake in formatting subgraphInit, shouldNotContain will 
>>>>>> not complain, and you will end up having a bad test that doesn't 
>>>>>> test what you're looking for.
>>>>>
>>>>> I actually have the reverse check initially, but decided to remove 
>>>>> it since we have other tests that explicitly check for archived 
>>>>> Long cache and system module subgraphs. I can add one test case 
>>>>> for the Long cache (for future proof in case the logging output is 
>>>>> changed).
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 11/21/18 2:17 PM, Jiangli Zhou wrote:
>>>>>>> Please review the following test fix in 
>>>>>>> ReplaceCriticalClasses.java. I re-implemented the subgraph check 
>>>>>>> to verify a subgraph is not used at runtime if one of it's klass 
>>>>>>> (who's object is contained within the archived subgraph) is 
>>>>>>> changed by the JVMTI agent.
>>>>>>>
>>>>>>> I added a test case for the newly archived Long cache subgraph. 
>>>>>>> Since both ResolvedModule and Long classes are loaded during the 
>>>>>>> primordial phase, '-early -notshared' are set in those test cases.
>>>>>>>
>>>>>>> As part of the change, I fixed 
>>>>>>> HeapShared::initialize_from_archived_subgraph to only do the 
>>>>>>> logging output after sbugraph(s) is successfully installed in 
>>>>>>> the given Klass.
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~jiangli/8214086/webrev.00
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8214086
>>>>>>>
>>>>>>> Tested ReplaceCriticalClasses.java locally on linux-x64. Will 
>>>>>>> run tier1-tier2.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>
>>>>
>>
>