RFR: 8186476: Generalize Atomic::add with templates
erik.osterlund at oracle.com
Mon Aug 21 13:10:45 UTC 2017
Thank you for finding an approach that seems to have reached consensus,
and rolling the ball forward while I was on vacation.
I agree this seems to be a reasonable way of doing things and am
delighted about the general approach.
When it comes to the proposed Atomic::add implementation, it seems to me
like the emulation of Atomic::add on jshort using jint is very similar
to how Atomic::cmpxchg on jbyte is emulated using jint. So it surprised
me a bit that Atomic::add of jshort does not share the same mechanism as
the generalized Atomic::cmpxchg of jbyte. In fact, it seems like the
proposed implementation does not safely handle CV-qualifiers. For
example, if the addend passed in is const qualified, I believe it will
not match the AddImpl<jshort, jshort> class specialization, as it is
looking for AddImpl<const jshort, jshort> instead.
Therefore, I would propose dealing with Atomic::add on jshort in one of
the following ways (numbered according to my preferences):
1) Remove it. Atomic::add on jshort is only ever used by a reference
counter in the Symbol class that can be placed next to the length. I
would be surprised if the memory savings of using short instead of int
for the reference counter is considerable.
2) Implement it like jbyte cmpxchg. That would entail both calling a
generalized AddShortUsingInt function object from the platform layer,
and preferrably also using an int CAS loop for emulating the add
operation rather than the current emulation using atomic int add, that
relies on two short fields being placed next to each other, depending on
endianness of the machine, using the ATOMIC_SHORT_PAIR macro for
declaring the short pair, that may or may not in fact enforce the
intended alignment at compile time.
3) Continue emulating jshort Atomic::add with jint Atomi::add, but turn
it into an AddShortUsingInt function object, called from the platform
layer, like the similar CmpxchgByteUsingInt operation.
So, I am curious if anyone would have loud objections if I was to remove
Atomic::add support for jshort, and changed its single use
(Symbol::_refcount), to use int instead. And if there are such
objections, I wonder if we really want to continue using the
ATOMIC_SHORT_PAIR macro and emulation using jint Atomic::add, or it is
okay to rewrite it to use an cmpxch loop instead and get rid of the
ATOMIC_SHORT_PAIR macro (that I find makes for a very weird API).
On 2017-08-20 08:19, Kim Barrett wrote:
>> On Aug 20, 2017, at 2:16 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> Please review this change to Atomic::add, making it a function
>> template with a per-platform underlying implementation.
>> This change is similar to 8186166: "Generalize Atomic::cmpxchg with
>> templates", with similar goals and taking a similar approach.
>> Ad hoc hotspot nightly test.
> I forgot to mention that this change is based on the 8186166 hotspot.04 change,
> which has not yet been pushed. I’m leaving that to Erik, as well as handing this
> change off to him.
More information about the hotspot-dev