review (S) for 6911204: generated adapters with large signatures can fill up the code cache

[Tom Rodriguez]

On Jan 13, 2010, at 8:48 PM, John Rose wrote:

> On Jan 13, 2010, at 4:09 PM, Tom Rodriguez wrote:
> 
>> http://cr.openjdk.java.net/~never/6911204
> 
> It looks good.
> 
> It would be good to include a comment about size statistics, so the future reader can get an idea about how common the compressed and uncompressed cases are.

I added this comment:

      // Storing the signature encoded as signed chars hits about 98%                                                                   
      // of the time.    

It's pretty much always the length limit that fails not the encoding size.

> 
> You might put the union first, before _length:
> 
> +   union {
> +     signed char  _compact[12];
> +     intptr_t* _fingerprint;
> +   } _value;
> +   int _length; // A negative length indicates that the _value is a C
> +                // heap allocated array.
> 
> That way on LP64 systems you won't get 4 dead bytes.

Ok.

> 
> This expression gave me the willies, even though it's correct.  It's best to parenthesize such things routinely:
> +       hash = hash << 7 ^ value(i);

I usually would tend to parenthesize that to make it clear.  I've added them.

> If it were me, I'd compress the series of reg values down to a series of bytes using CompressedWriteStream::write_signed_int, and just do hashes and equality checks on the actual compressed bytes.  There'd be no need for value().  The bytes would never be decoded except for debugging routines like as_string.  The union trick would be the same, except that the pointer would point to an array of chars.  The actual equality comparisons could be done with memcmp.
> 
> But it doesn't make a big difference, since this isn't hot code.

It sort of is hot.  It's called to link every methodOop since every one has to have an adapter.  Mostly it hits the cache without having to generate new adapters.  For jbb it's something like 10000 calls with about 150 adapters actually generated.  Prior to this change we generated about 600 adapters of which 13 where the -1 signature so they couldn't be reused.  
Using the calling convention instead of the fingerprint is slightly more expensive than the fingerprint which I don't love but I think it's acceptable.  CompressedWriteStream won't necessarily work since VMReg is really an encoded pointer, though I don't think we ever use values outside of the 32 bit int space.

Anyway, I guess in the end I think the space required for this is relatively small given the number of adapters normally required.  If anything I think my current attempt to space might be over complicated.  I think I'll just leave it as is.

I've updated the webrev.

tom

> 
> -- John