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

[Thomas Schatzl]

Hi,

On Thu, 2013-05-16 at 16:19 +0200, Bengt Rutisson wrote:
> 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?

No. If you use a different GC than G1 it does not matter, you have the
"memory" leak I was talking about.

If it is G1, the next time a PerRegionTable is allocated, it will be
reinitialized with a correct HeapRegion pointer anyway.

> 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 do not think there is a risk as the allocating thread is responsible
for using/not using any method of the PerRegionTable that uses the
HeapRegion reference. As soon as the PerRegionTable is allocated
(owned), no other owner can access it.

I had the impression that these unit tests run before control is passed
to the java program. So at this point there can be no one trying to mess
around with the free list.

You are right that it relies on knowledge of the internals of the
alloc() method and the constructor of the PerRegionTable, but I do not
see a way to avoid it. We are somewhat tied to the class by the single
static free list pointer.

Except maybe extracting the free list management into a class,
instantiate a test private instance of it, add setup/teardown methods
for both the free list management and the PerRegionTable instance (the
problem is, this leads to changes up to the BitMap class...).

At some point we may even want to do that because maybe there is too
much contention in free list management. It seems a lot of effort for
too little gain. I can do that if you want.

Maybe you have a better idea?

> 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 

Okay, will do.

> 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?

You mean PerRegionTable::test_fl_mem_size()? I will do that, but I
thought at the end of the class would be sufficient.

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

Thanks for your input,
Thomas