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

[Bengt Rutisson]

Hi Thomas,

On 5/16/13 5:00 PM, Thomas Schatzl wrote:
> 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.

Right. But the memory leak does not worry me too much. It is small and 
limited.

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

Yes, this is what worries me a bit. You add a "dummy" PRT to the free 
list. Then when G1 starts running and needs a PRT this dummy PRT will be 
the first one to be used. It will be re-initialized in 
PerRegionTable::alloc(), as you say, but this seems scary to me. The 
test manipulates the data structures that we will use at runtime. I 
would much prefer that the test cleaned up after itself. Maybe something 
like:

void PerRegionTable::test_fl_mem_size() {
   PerRegionTable* dummy = alloc(NULL);
   free(dummy);
   guarantee(dummy->mem_size() == fl_mem_size(), "fl_mem_size() does not 
return the correct element size");

   _free_list = NULL;
   delete dummy;
}

I realize that this will still leak the memory for the bitmap allocated 
by the PRT since there is no decent destructor for the PRT, but as I 
said I'm less worried about leaking memory than putting the VM in a 
strange state because of the test.

I don't think we need the check for G1 that you added in the second 
review request. It is more important that we make sure that this test is 
run than that we avoid a small memory leak.

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

Yes, it is this internal knowledge that worries me. It does not seem 
unreasonable for someone to add a check in methods like alloc() or 
free() that checks that the new or old HR is consistent in some way.

However, there are no such checks now, so I guess it is fine to use NULL.

>
> 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 don't have any really simple ideas. It is very difficult to test 
classes that use static data. So, I agree that to be able to write nice 
tests we need to refactor the code quite a lot. Probably not worth it 
for now.

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

I looked at your second webrev and you have addressed these comments. 
Thanks!

Bengt

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