RFR: 8024671: G1 generates assert error messages in product builds
thomas.schatzl at oracle.com
Thu Sep 12 13:13:09 UTC 2013
On Thu, 2013-09-12 at 15:09 +0200, Bengt Rutisson wrote:
> Hi Aleksey and Thomas,
> On 9/12/13 2:40 PM, Thomas Schatzl wrote:
> > Hi,
> > On Thu, 2013-09-12 at 16:37 +0400, Aleksey Shipilev wrote:
> >> On 09/12/2013 04:30 PM, Thomas Schatzl wrote:
> >>> thanks for finding and fixing this. Also for catching the typo.
> >> Thanks for the review, Thomas!
> >>> On Thu, 2013-09-12 at 15:59 +0400, Aleksey Shipilev wrote:
> >>>> Hi,
> >>>> Please review and push this simple change:
> >>>> http://cr.openjdk.java.net/~shade/8024671/webrev.00/
> >>>> TL;DR: err_msg() is not getting erased in product builds, because
> >>>> check_card_num is not a macro, causing the performance regression. Turns
> >>>> out it is simpler to inline the check_card_num all along, because it is
> >>>> more consistent to other asserts.
> >>> I really do not like the inlining of the check_card_num() method,
> >>> replicating the code. I would prefer an approach that avoids that.
> >> I thought that too before starting the patch, targeting to declare the
> >> proper macro for check_card_num. But, inline asserts seem be coherent
> >> will other asserts in the affected code blocks. Hence, I changed my
> >> mind, and produced the current patch.
> > we can always improve the code :)
> > Also the use of check_card_num() has probably been an attempt to factor
> > out the common checking code, although it had the performance side
> > effects you found.
> I think I prefer the inlined versions as suggested in this patch. It is
> only one line and this way it is very fast to see what is actually being
> checked. Defining a macro or something will be less readable in my opinion.
As mentioned, if you are okay with that, it's fine with me.
> Thomas, if you are ok with the current patch I can sponsor the push.
> Aleksey, I'll wait with doing the actual push until tomorrow to allow
> people in other time zones to have a look before this gets pushed. If
> there are no objections until tomorrow morning I'll start the push job.
More information about the hotspot-gc-dev