RFR: JDK-8016309 - assert(eden_size > 0 && survivor_size > 0) failed

John Coomes John.Coomes at oracle.com
Fri Nov 1 16:00:53 UTC 2013


Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
> Hi John,
> 
> Indeed there was a part missing from the webrev. The webrev script didn't pick 
> up all patches in my mercurial queue. I made a new webrev here:
> 
> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.5/
> ...

Looks good to me. :-)

-John

> John Coomes skrev 1/11/13 2:34 PM:
> > Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
> >> Hi John,
> >>
> >> Thanks for the review!
> >>
> >> I have fixed the things you commented on. Rather than making a new webrev for
> >> the entire change I made one to put on top of the previous change. This way you
> >> can see exactly what I have changed this time without having to look through all
> >> of it again.
> >
> > Much easier that way, thanks.
> >
> >> The new patch:
> >>
> >> http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.4/
> >>
> >> Comments inline.
> >
> > Looks good, except something seems to be missing from the webrev.  See
> > comments below.
> >
> >> John Coomes skrev 30/10/13 6:28 PM:
> >>> Jesper Wilhelmsson (jesper.wilhelmsson at oracle.com) wrote:
> >>>> Hi Thomas,
> >>>>
> >>>> Thanks for looking at this!
> >>>>
> >>>> Comments inline.
> >>>>
> >>>> ...
> >>>> A new webrev is available now with changes based on your comments and comments
> >>>> from John Coomes. Right now it applies cleanly to hotspot-gc. The bugfix builds
> >>>> on top of the cleanup, so apply the cleanup first. The cleanup is unchanged and
> >>>> has gotten sufficient reviews by now.
> >>>>
> >>>> Cleanup: http://cr.openjdk.java.net/~jwilhelm/8016309/Cleanup/webrev.3/
> >>>>
> >>>> Bugfix:  http://cr.openjdk.java.net/~jwilhelm/8016309/Bugfix/webrev.3/
> >>>
> >>> Hi Jesper,
> >>>
> >>> Generally looks good; some comments below.
> >>>
> >>> collectorPolicy.hpp:
> >>>
> >>> 	default_gen_alignment() is 64K heapwords, which was the value
> >>> 	from the old ParallelScavengeHeap::intra_heap_alignment().  The
> >>> 	latter applied only to ParallelScavengeHeap, but the former now
> >>> 	applies to CMS and SerialGC and G1, so the alignment for those
> >>> 	GCs has been increased unnecessarily.  There should instead be a
> >>> 	specialized default_gen_alignment() for ParallelScavenge (in
> >>> 	GenerationSizer) with the larger value.
> >>
> >> G1 has its own initialize_alignments so it should be good. For the others, the
> >> changes in the patch will allow CMS and Serial to keep using the smaller alignment.
> >
> > I see the new default_gen_alignment() in GenerationSizer, which is
> > fine.  But the webrev doesn't show what happened to the old
> > implementation in collectorPolicy.hpp - it's not there anymore, but
> > there's no diff showing that it was removed.
> >
> >>> 	The assert_*_info() methods should be wrapped in DEBUG_ONLY() to
> >>> 	ensure there's no code in the product vm.
> >>
> >> How much do we trust the compilers to remove unused code? In the patch I have
> >> wrapped the whole methods in DEBUG_ONLY(), but if we think that the compilers
> >> will remove dead code it would be enough to wrap the calls.
> >> Wrapping the whole methods will however allow the compiler to make sure they are
> >> not called in a product build.
> >
> > Wrapping the calls only would be fine, except you have to make sure
> > that you find all of them, and you don't get help from the compiler to
> > do it :-(.
> >
> > Also, the usual convention has been to use DEBUG_ONLY() for single
> > lines of code, and
> >
> > 	#ifdef ASSERT
> > 	...
> > 	#endif // ASSERT
> >
> > for larger blocks (like whole methods, or groups of methods).  What
> > you have is ok, but it does make it hard for some editors to do proper
> > indentation.
> >
> > -John
> >
> >>>
> >>> collectorPolicy.cpp:
> >>>
> >>> - GenCollectorPolicy::initialize_flags()
> >>>
> >>> 	If NewSize > MaxNewSize, the old code would increase MaxNewSize;
> >>> 	the new code will exit w/an error.  That behavior shouldn't be
> >>> 	changed as part of this bug fix, and not this late in jdk8.
> >>
> >> Sure, I'll change that back. I do think we should try to be less forgiving when
> >> the user sets conflicting flags though. (This exit only happens if the user has
> >> set both NewSize and MaxNewSize.)
> >>
> >>> 	There should be an assert that
> >>> 		 _heap_alignment % _gen_alignment == 0
> >>> 	either here or in assert_size_info().
> >>
> >> Agreed. Fixed.
> >>
> >>>
> >>> 	Syntax nits:
> >>>
> >>> 	There are several local vars with camel-case names that should
> >>> 	be lower-case with underscores (e.g., smallestNewSize should be
> >>> 	smallest_new_size)
> >>
> >> OK. Fixed.
> >>
> >>> 	The method is big now and should be split (ok to do later, if
> >>> 	necessary).
> >>
> >> There are a few more cleanups of these classes planned so I'll put that on my
> >> todo list.
> >>
> >>>
> >>> generationSizer.hpp:
> >>>
> >>> 	You've made initialize_size_info, initialize_flags() and
> >>> 	initialize_size_info() private.  They're protected in
> >>> 	superclasses.
> >>
> >> Oops. Fixed.
> >>
> >>> 	I think a generationSizer.cpp is needed, instead of making these
> >>> 	methods inline (they're not relevant to performance).
> >>
> >> OK. Done.
> >>
> >>> 	I don't like that after the page size is selected in
> >>> 	GenerationSizer::initialize_size_info(), the entire sizing
> >>> 	calculation is redone.  It's partially an artifact of the
> >>> 	structure of the old code, so I'm ok with it for now as long as
> >>> 	it's revisited later (but please file an issue).
> >>
> >> I agree and I have an idea on how to do it better. I have created an RFE to fix
> >> this: https://bugs.openjdk.java.net/browse/JDK-8027649
> >>
> >> Thanks!
> >> /Jesper
> >>
> >>>
> >>> -John
> >>>



More information about the hotspot-gc-dev mailing list