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

[Thomas Schatzl]

Hi,

On Fri, 2013-05-17 at 08:04 +0200, Bengt Rutisson wrote:
> 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 

The test already manipulates the free list pointer.

> would much prefer that the test cleaned up after itself. Maybe something 

Me too :)

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

Okay, I don't mind that way, although as mentioned it does not seem to
be much different.

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

I will remove the check then. It was a suggestion.

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

The problem I see is that for a real proper, independent test we would
need to mock an entire G1CollectedHeap and associated data structures
(e.g. policy, HeapRegions, etc).

There is no infrastructure for doing so at all. I believe this
definitely extends the scope of this CR :)

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

I will send another later today addressing your comments. Thanks.

Thomas