On Fri, 1 Jul 2022 18:36:26 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Zhengyu Gu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Incorporated plummercj's test changes
>
> test/jdk/com/sun/jdi/TestClassUnloadEvents.java line 61:
>
>> 59: return;
>> 60: }
>> 61: }
>
> I know you were having issues with the test sometimes failing and asked about
> if the GC's are deterministic, I assume this is the work around for that. My
> concern is if something were to change and the test started to never get any
> ClassUnloadEvents, that would go unnoticed. I'd rather you made it so the
> test failed after exiting this loop, and bump up MAX_RETRY to a high enough
> number so that we are unlikely to ever see that happen.
>
> You should also have a look at the runtime/ClassUnload/UnloadTest.java and
> see how it handles class unloading. It relies on the ClassUnloadCommon
> library class. One difference I see there is that before doing a System.gc(),
> it first allocates 16m of memory in 1k chunks to force a young gen
> collection. I don't understand details of GC collection, but perhaps
> System.gc() is simply promoting from the youngGen and therefore not freeing
> any Class instance found there, so you need to promote first and then do the
> full GC.
Changed to use `ClassUnloadCommon` for triggering class unloading.
> test/jdk/com/sun/jdi/TestClassUnloadEvents.java line 182:
>
>> 180: eventSet.resume();
>> 181: }
>> 182: } catch (InterruptedException | VMCannotBeModifiedException |
>> IOException | IllegalConnectorArgumentsException | VMStartException e) {
>
> I think you should remove the try/catch and just declare that this method
> throws Exception. That seems to be what other tests are doing.
Done.
-------------
PR: https://git.openjdk.org/jdk/pull/9168
