RFR (XS): 8014405: G1: PerRegionTable::fl_mem_size() calculates size of the free list using wrong element sizes
bengt.rutisson at oracle.com
Thu May 16 14:19:35 UTC 2013
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!
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.
> jprt (which runs the unit tests)
> The main patch is the one-liner in heapRegionRemSet.cpp,
> 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()
> 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.
More information about the hotspot-gc-dev