RFR (XS): 8211123: GC Metaspace printing after full gc does not reflect recent changes (Was: Re: A Bug Of Metaspace After Full GC)

[Thomas Stüfe]

Hi JC,

On Thu, Sep 27, 2018 at 8:29 PM, JC Beyler <jcbeyler at google.com> wrote:
> Hi Thomas and Thomas,
>
> Thanks for the reviews!
>
> @Thomas Schatzl: I looked for using a MXBean but I'm confused about using
> this. What this fix+test is trying to ascertain is that the moment of the
> logging of the metaspace data, the new size has been calculated; how do I do
> that with a MXBean? Would I not always get the current size? So if I did
> before FullGC, I'd get the previous size, after FullGC, I'd always get the
> new size, no? I am most likely missing something and I apologize!
>
> @Thomas Stufe: I think that is exactly what we could (should?) use; however
> there are a few questions here:
>    1) There is no license to that code, could you add one so that we can see
> if it is compatible for the OpenJDK project?

There was none? Oops. Sure, I added a license.

>    2) Will bringing this code in for this test be useful for future/other
> tests?

I think so (but not in its current form, these were just quick and
dirty tests to trigger a particular pathological OOM scenario - they
do what they do but they could be prettier).

When I did JDK-8198423, I added tests at C++ level as gtests, see
test_metaspace_allocation.cpp. I felt I had more control at that
level. However, I still feel I owe the project some nice java level
tests, plus, it would be nice to have some metaspace stress scenarios
to play around.

>    3) If so, I would actually recommending making a RFE for the test to
> bring in similar code like that and remove my testcase.jar. That way this
> bug focuses solely on just getting the print out right with a test and we
> can refactor it in a second step with a useful common library that can come
> it slowly with reviews that are concentrating on that library instead, what
> do you think?

Sounds good.

Thanks, Thomas

>
> Thanks,
> Jc
>
> On Thu, Sep 27, 2018 at 7:17 AM Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
>>
>> Hi,
>>
>> On Thu, Sep 27, 2018 at 2:09 PM, Thomas Schatzl
>> <thomas.schatzl at oracle.com> wrote:
>> > Hi JC,
>> >
>> > On Wed, 2018-09-26 at 19:21 -0700, JC Beyler wrote:
>> >> Hi Thomas et al,
>> >>
>> >> Thanks for moving this thread to a RFR. Here is the webrev and bug:
>> >> Webrev: http://cr.openjdk.java.net/~jcbeyler/8211123/webrev.00/
>> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8211123
>> >>
>> >> For the test, I inspired myself of a few different tests to get this
>> >> up and running. I didn't find a means to use only the WhiteBox to get
>> >> the metaspace filled. I did not want to use the .jar that the
>> >> original reproducer was using since it was using a library. So I used
>> >> the testcases.jar from the runtime/7116786 test.
>> >>
>> >> I also use a separate process to test the various GCs and ensure it
>> >> works for all.
>> >>
>> >> Btw, I've put nijiaben as the contributor for the webrev.
>> >>
>> >> Let me know what you think, I imagine I've made the test a bit more
>> >> complicated than need be,
>> >> Jc
>> >
>> > I do not know a better way than having an external jar to do class
>> > loading into a separate class loader in a test. It's safest to
>> > duplicate the jar file.
>>
>> Just as a suggestion, I did tests - but never really got to jtreg-ify
>> them - for JDK-8198423.
>>
>> They use javax.tools.JavaCompiler to compile classes on the fly and
>> load them into separate class loaders.
>>
>> See e.g.
>> https://github.com/tstuefe/ojdk-repros/blob/master/src/test3/Example2.java
>>
>> (Not the prettiest program ever, but enough to demonstrate the point).
>>
>> Best, Thomas
>>
>> >
>> > The test could be simplified a bit by using the Metaspace MemoryMXBean
>> > to get metaspace memory readings (there is an MXBean called
>> > "Metaspace"), removing the need for invoking a secondary JVM in the
>> > test and parsing the log :)
>> >
>> > You can then run the test with the different collectors using
>> > "main/othervm -XX:+<some-gc> MyTest" lines.
>> >
>> > Otherwise the change itself looks good. I would appreciate if the test
>> > could be improved as mentioned above, but if you do not want to I could
>> > live with it as is.
>> >
>> > Btw, you can have multiple contributors in that line (or probably
>> > multiple Contributed-by lines).
>> >
>> > Thanks,
>> >   Thomas
>> >
>> >
>
>
>
> --
>
> Thanks,
> Jc