RFR: 8013155: [pack200] improve performance of pack200

[John Rose]

On Apr 30, 2013, at 11:32 AM, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:

> Couple of nits:
> I don't think you need the parens
> 
> j = (nextsemi < nextangl ? nextsemi : nextangl);

I recommend (and many style guides recommend) adding extra parens in when the operator precedence is obscure.  In this case, assignment and ?: are competing for precedence, so the parens serve a purpose.  (At least they do for this reader of code.)

> Coding conventions nits:
> missing spaces for operators.
> 
> for (int i = firstl+1, j; i > 0; i = sig.indexOf('L', j)+1) {

I agree.

The old code was bad for two reasons:

1. It had a String.intern in it; those are usually a bottleneck
2. It looped over strings using String.charAt.

I'm kind of sad (as a JIT guy) that the optimizer didn't unwind all the charAt overhead.  The JIT works hard on array range checks but apparently it is not as effective on the explicitly coded range checks inside String.  We should watch that.  I filed a tracking bug:

  8013655: external loop over String characters has extra overhead

The new code avoids that overhead by using the canned loops in String.indexOf, which work on the naked array.

Thanks for tracking this down.

— John