RFR (XS): 8014405: G1: PerRegionTable::fl_mem_size() calculates size of the free list using wrong element sizes

[Bengt Rutisson]

Hi Thomas,

The code change looks good.

I'm a little concerned about the test. Isn't there a risk that the VM 
will crash if it keeps running after the tests are complete since you 
have added the dummy PerRegionTable to the free list?

It also seems a bit risky to use NULL for the heap region to pass to 
alloc(). It works since no one checks for NULL or tries to use it, but 
it feels a bit unstable.

I think the test() method could have a more descriptive name. Maybe 
test_fl_mem_size()? Finally, I think most other modules that use the 
ExecuteInternalVMTests "framework" add the test methods to a section at 
the bottom of the file. Do you think it is worth following this 
convention and moving your test method to the end of the file?

Having said this, I am really happy that you added a test for this! 
Thanks for doing that!

Bengt

On 5/15/13 10:22 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for the following change?
>
> It fixes the memory usage calculation for the G1 fine remembered set
> free list. Previously it summed up only sizes of the PerRegionTable
> object instances, missing out on memory allocated on the heap.
>
> bugs.sun.com:
> http://bugs.sun.com/view_bug.do?bug_id=8014405
>
> JIRA:
> https://jbs.oracle.com/bugs/browse/JDK-8014405
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8014405/webrev/
>
> Testing:
> jprt (which runs the unit tests)
>
> The main patch is the one-liner in heapRegionRemSet.cpp,
> PerRegionTable::fl_mem_size().
>
> The other changes add support code for unit testing: note that since
> PerRegionTable is a private class, and I preferred to not make it
> public, the test invocation is funneled through the new
> HeapRegionRemSet::test_prt() method. Which I think is okay, since
> PerRegionTable is a component of HeapRegionRemSet.
>
> This test method is then invoked by the code in jni.cpp.
>
> The test itself is straightforward: allocate a PerRegionTable instance,
> free it to put it on the free list, and compare the sizes reported by
> the PerRegionTable instance and the PerRegionTable::fl_mem_size()
> method.
> Some nits: Since there is no way to clean up a PerRegionTable instance
> (and the original code never does), if you run the unit tests, this may
> be considered a memory leak.
> Properly cleaning up would require quite a few additions to other
> classes though so I did not implement it just for the test. If you
> consider this important, I will add this, but it seemed that this added
> a lot of noise to the one-line patch.
>
> Thanks,
>    Thomas
>
>