RFR (S) 8009575 (2nd) - Reduce Symbol::_refcount from 4 bytes to 2 bytes
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Jun 21 11:19:15 PDT 2013
On 6/20/13 6:39 PM, Coleen Phillimore wrote:
> On 06/20/2013 08:27 PM, David Holmes wrote:
>> Hi Ioi,
>> Reviewed. But see below.
>> On 21/06/2013 4:20 AM, Ioi Lam wrote:
>>> Please review my 2nd trial for this bug fix. It's much simplified
>>> to Coleen's suggestion.
>>> Bug: Reduce Symbol::_refcount from 4 bytes to 2 bytes
>>> Summary of fix:
>>> As noted in the bug report, the main problem is _refcount needs
>>> to be
>>> atomically incremented, but the smallest unit that
>>> Atomic::incr() had
>>> supported until now was 32-bit int.
>>> With Coleen's help, I have created a platform-independent version
>>> of Atomic::incr(short*) that's based on Atomic::add(int*, n).
>>> Essentially, Atomic::incr(short*) can be implemented as
>>> Atomic::add(int*, 0x10000), as long as the short occupies the most
>>> significant 16 bits of the int. I added the macro
>>> to ensure the proper alignment.
>> I preferred the original version that constrained this to
>> symbol.hpp/cpp. We don't really support Atomic jshort operations in a
>> general sense - we only have inc/dec and even then this only applies
>> when you have the pair of shorts with only one needing atomic access!
>> So I would have preferred not to see this exposed in the Atomic class.
> Hi David,
> I had opposite opinion and urged Ioi to hide the atomic short
> operations in the atomic class. It might be of general use and he
> implemented it with the minimal implementation details to distract the
> Symbol class. I have reviewed and like this change. It is really
> clean and doesn't expose endianness where it isn't interesting.
I'll have to concur that I think the ENDIAN-ness is encapsulated in
the right place in this version... Now if I can only come up with
something as clean when I get back to padding in monitors/locks...
>> Minor comment: are the friend declarations needed for both Symbolbase
>> and Symbol?
> Looks like a lot of friendliness.
>>> Also Symbol::size(int) is fixed to calculate the correct space
>>> for a Symbol. (Same as in the previos webrev --
>>> - vm.tmtools.testlist nsk.sajdi.testlist vm.runtime.testlist
>>> vm.quick.testlist vm.parallel_class_loading.testlist
>>> - Serviceability Agent tested are included in these lists.
>>> - Ioi
More information about the hotspot-runtime-dev