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

[Thomas Schatzl]

Hi,

  there is a new webrev at
http://cr.openjdk.java.net/~tschatzl/8014405/webrev.1/

Thanks for your previous review btw.

The changes are:
- renamed PerRegionTable::test() to PerRegionTable::test_fl_mem_size()
- moved the test method implementation to the bottom of the file.
- fixed the "memory leak" when not using g1 I was talking about by
simply checking whether G1 is enabled or not. If it is not, just return.

I left the test case as is for now, awaiting your comments.

Testing:
jprt

Hth,
  Thomas

On Thu, 2013-05-16 at 17:00 +0200, 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.
> 
> 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
> 
>