RFR (XS): 8021823 G1: Concurrent marking crashes with -XX:ObjectAlignmentInBytes>=32 in 64bit VMs (patch for hs24+hs25)

[Thomas Schatzl]

Hi,

On Wed, 2013-07-31 at 10:29 -0700, Vladimir Kozlov wrote:
> The fix looks good.
> 
> About test. You don't need "@run main/othervm TestObjectAlignment" line. 
> First, you don't need /othervm since you don't use any options with main 
> test. Second, as was explained me recently, jtreg will execute simple 
> "@run main TestObjectAlignment" by itself so you don't need to specify it.
> 

Changed; I do not like this automatism as I've been tripped over it
least once (e.g. as soon as you add a masked @run statement like @build,
the test is not automatically run any more), so I left the @run
statement.

You are correct though about both issues, and I do not really have a
strong opinion about this. I've updated the webrevs at

hs24: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.2.hs24/
hs25: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.2.hs25/

Thanks,
Thomas

> Regards,
> Vladimir
> 
> On 7/31/13 10:05 AM, Thomas Schatzl wrote:
> > Hi,
> >
> > On Wed, 2013-07-31 at 08:54 -0700, Vladimir Kozlov wrote:
> >> Thomas,
> >>
> >> There is LogMinObjAlignment. Why you did not use it?
> >
> >    did not recall it at that time it for some reason...
> >
> > New Webrevs:
> >
> > hs24: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.1.hs24/
> > hs25: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.1.hs25/
> >
> > Fixed now, thanks,
> >    Thomas
> >
> >>
> >> Thanks,
> >> Vladimir
> >>
> >> On 7/31/13 5:48 AM, Thomas Schatzl wrote:
> >>> Hi all,
> >>>
> >>>     can I have reviews for the following fix? The change addresses wrong
> >>> initialization of the mark bitmaps in G1 when ObjectAlignmentInBytes is
> >>> larger than or equal to 32.
> >>>
> >>> The concurrent marking uses a shift value for mapping heap address to
> >>> mark bitmap bits. This shift value has been calculated wrongly depending
> >>> on ObjectAlignment (=ObjectAlignmentInBytes in HeapWords), so that only
> >>> worked for ObjectAlignmentInBytes values <= 16 it has been correct.
> >>>
> >>> Instead of the correct value of "log2(ObjectAlignment)" the code used
> >>> "ObjectAlignment-1".
> >>>
> >>> I.e. some table will show the problem (64 bit VM with sizeof(HeapWord)
> >>> == 8):
> >>>
> >>> ObjectAlignment (OAInBytes)  Old Shift value  New Shift value
> >>>          8 (64)                      7                3
> >>>          4 (32)                      3                2
> >>>          2 (16)                      1                1
> >>>          1 (8)                       0                0
> >>>
> >>> I.e. when mapping HeapWords to bits, the code previously used a much too
> >>> high value for ObjectAlignmentInBytes, resulting in truncating important
> >>> bits from the heap address, resulting in wrong locations to look for and
> >>> set bits.
> >>>
> >>> Coincidently the old calculation resulted in the same values for
> >>> ObjectAlignmentInBytes<=16, so this issue went unnoticed.
> >>>
> >>> There is a new test that simply looks for these crashes for all
> >>> supported object alignments by running a few concurrent markings.
> >>>
> >>> JBS:
> >>> https://jbs.oracle.com/bugs/browse/JDK-8021823
> >>>
> >>> Bugs.sun:
> >>> http://bugs.sun.com/view_bug.do?bug_id=8021823
> >>>
> >>> Webrev:
> >>> hs24: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.hs24/
> >>> hs25: http://cr.openjdk.java.net/~tschatzl/8021823/webrev.hs25/
> >>>
> >>> Testing:
> >>> manual testing of all failing tests with ObjectAlignmentInBytes from 8
> >>> to 256, test case failing without the patch, succeeding with the patch,
> >>> jprt.
> >>>
> >>> Testing:
> >>>
> >
> >