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

[Tom Rodriguez]

It's hot but not necessarily the most expensive.  We have to parse the UTF8 signature to construct an array of BasicTypes and then pass that to the calling_convention to build a VMRegPair[].  That's an interesting trick to speed up the equals test itself though.  I discovered a problem with the hash computation for the compacted form that's leading me to reevaluate the hash function itself and collect some statistics.  I'm going to fix that and maybe incorporate your faster equals.

tom

On Jan 15, 2010, at 11:15 AM, Vladimir Kozlov wrote:

> Do I understand correctly that hottest method is equals()?
> If so you can optimized it for signed chars:
> 
> +   union {
> +     signed char  _compact[12];
> +     int          _compact_int[3];
> +     intptr_t* _fingerprint;
> +   } _value;
> 
> +       // Initialize array
> +       _compact_int[0] = 0;
> +       _compact_int[1] = 0;
> +       _compact_int[2] = 0;
> +
> +       for (int i = 0; i < total_args_passed; i++) {
> +         VMRegPair pair = regs[i];
> +         intptr_t v1 = pair.first()->value();
> +         intptr_t v2 = pair.second()->value();
> +         if (v1 == (signed char) v1 &&
> +             v2 == (signed char) v2) {
> +           _value._compact[o++] = v1;
> +           _value._compact[o++] = v2;
> +         } else {
> +           goto big;
> +         }
> +       }
> 
> 
> +   bool equals(AdapterFingerPrint* other) {
> +     if (other->_length != _length) {
> +       return false;
> +     }
> +     if (_length  < 0) {
> +       return (other->_value._compact_int[0] == value._compact_int[0]) &&
> +              (other->_value._compact_int[1] == value._compact_int[1]) &&
> +              (other->_value._compact_int[2] == value._compact_int[2]);
> +     }
> +     for (int i = 0; i < length(); i++) {
> +       if (other->value(i) != value(i)) {
> +         return false;
> +       }
> +     }
> +     return true;
> +   }
> 
> Vladimir
> 
> Tom Rodriguez wrote:
>>> 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