RFR (L, but tedious) 8188220: Remove Atomic::*_ptr() uses and overloads from hotspot
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Oct 16 13:27:24 UTC 2017
On 10/15/17 9:18 PM, David Holmes wrote:
> One tiny follow up as I was looking at this code:
>
> src/hotspot/share/services/mallocSiteTable.hpp
>
> 65 MallocSiteHashtableEntry* _next;
>
> should be
>
> 65 MallocSiteHashtableEntry* volatile _next;
>
> as we operate on it with CAS.
Ok, got it. thanks.
Coleen
>
> Thanks,
> David
>
> On 14/10/2017 10:32 PM, David Holmes wrote:
>> Hi Coleen,
>>
>> These changes all seem okay to me - except I can't comment on the
>> Atomic::sub implementation. :)
>>
>> Thanks for adding the assert to header_addr(). FYI from
>> objectMonitor.hpp:
>>
>> // ObjectMonitor Layout Overview/Highlights/Restrictions:
>> //
>> // - The _header field must be at offset 0 because the displaced header
>> // from markOop is stored there. We do not want markOop.hpp to include
>> // ObjectMonitor.hpp to avoid exposing ObjectMonitor everywhere. This
>> // means that ObjectMonitor cannot inherit from any other class nor
>> can
>> // it use any virtual member functions. This restriction is
>> critical to
>> // the proper functioning of the VM.
>>
>> so it is important we ensure this holds.
>>
>> Thanks,
>> David
>>
>> On 14/10/2017 4:34 AM, coleen.phillimore at oracle.com wrote:
>>>
>>> Hi, Here is the version with the changes from Kim's comments that
>>> has passed at least testing with JPRT and tier1, locally. More
>>> testing (tier2-5) is in progress.
>>>
>>> Also includes a corrected version of Atomic::sub care of Erik
>>> Osterlund.
>>>
>>> open webrev at
>>> http://cr.openjdk.java.net/~coleenp/8188220.kim-review-changes/webrev
>>> open webrev at
>>> http://cr.openjdk.java.net/~coleenp/8188220.review-comments/webrev
>>>
>>> Full version:
>>>
>>> http://cr.openjdk.java.net/~coleenp/8188220.03/webrev
>>>
>>> Thanks!
>>> Coleen
>>>
>>> On 10/13/17 9:25 AM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> Hi Kim, Thank you for the detailed review and the time you've spent
>>>> on it, and discussion yesterday.
>>>>
>>>> On 10/12/17 7:17 PM, Kim Barrett wrote:
>>>>>> On Oct 10, 2017, at 6:01 PM, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>> Summary: With the new template functions these are unnecessary.
>>>>>>
>>>>>> The changes are mostly s/_ptr// and removing the cast to return
>>>>>> type. There weren't many types that needed to be improved to
>>>>>> match the template version of the function. Some notes:
>>>>>> 1. replaced CASPTR with Atomic::cmpxchg() in mutex.cpp,
>>>>>> rearranging arguments.
>>>>>> 2. renamed Atomic::replace_if_null to Atomic::cmpxchg_if_null. I
>>>>>> disliked the first name because it's not explicit from the
>>>>>> callers that there's an underlying cas. If people want to fight,
>>>>>> I'll remove the function and use cmpxchg because there are only a
>>>>>> couple places where this is a little nicer.
>>>>>> 3. Added Atomic::sub()
>>>>>>
>>>>>> Tested with JPRT, mach5 tier1-5 on linux,windows and solaris.
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8188220.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8188220
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>> I looked harder at the potential ABA problems, and believe they are
>>>>> okay. There can be multiple threads doing pushes, and there can be
>>>>> multiple threads doing pops, but not both at the same time.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/cpu/zero/cppInterpreter_zero.cpp
>>>>> 279 if (Atomic::cmpxchg(monitor, lockee->mark_addr(), disp)
>>>>> != disp) {
>>>>>
>>>>> How does this work? monitor and disp seem like they have unrelated
>>>>> types? Given that this is zero-specific code, maybe this hasn't been
>>>>> tested?
>>>>>
>>>>> Similarly here:
>>>>> 423 if (Atomic::cmpxchg(header, rcvr->mark_addr(), lock)
>>>>> != lock) {
>>>>
>>>> I haven't built zero. I don't know how to do this anymore (help?)
>>>> I fixed the obvious type mismatches here and in
>>>> bytecodeInterpreter.cpp. I'll try to build it.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/asm/assembler.cpp
>>>>> 239 dcon->value_fn = cfn;
>>>>>
>>>>> Is it actually safe to remove the atomic update? If multiple threads
>>>>> performing the assignment *are* possible (and I don't understand the
>>>>> context yet, so don't know the answer to that), then a bare
>>>>> non-atomic
>>>>> assignment is a race, e.g. undefined behavior.
>>>>>
>>>>> Regardless of that, I think the CAST_FROM_FN_PTR should be retained.
>>>>
>>>> I can find no uses of this code, ie. looking for "delayed_value". I
>>>> think it was early jsr292 code. I could also not find any
>>>> combination of casts that would make it compile, so in the end I
>>>> believed the comment and took out the cmpxchg. The code appears
>>>> to be intended to for bootstrapping, see the call to
>>>> update_delayed_values() in JavaClasses::compute_offsets().
>>>>
>>>> The CAST_FROM_FN_PTR was to get it to compile with cmpxchg, the new
>>>> code does not require a cast. If you can help with finding the
>>>> right set of casts, I'd be happy to put the cmpxchg back in. I just
>>>> couldn't find one.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/classfile/classLoaderData.cpp
>>>>> 167 Chunk* head = (Chunk*) OrderAccess::load_acquire(&_head);
>>>>>
>>>>> I think the cast to Chunk* is no longer needed.
>>>>
>>>> Missed another, thanks. No that's the same one David found.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/classfile/classLoaderData.cpp
>>>>> 946 ClassLoaderData* old = Atomic::cmpxchg(cld, cld_addr,
>>>>> (ClassLoaderData*)NULL);
>>>>> 947 if (old != NULL) {
>>>>> 948 delete cld;
>>>>> 949 // Returns the data.
>>>>> 950 return old;
>>>>> 951 }
>>>>>
>>>>> That could instead be
>>>>>
>>>>> if (!Atomic::replace_if_null(cld, cld_addr)) {
>>>>> delete cld; // Lost the race.
>>>>> return *cld_addr; // Use the winner's value.
>>>>> }
>>>>>
>>>>> And apparently the caller of CLDG::add doesn't care whether the
>>>>> returned CLD has actually been added to the graph yet. If that's not
>>>>> true, then there's a bug here, since a race loser might return a
>>>>> winner's value before the winner has actually done the insertion.
>>>>
>>>> True, the race loser doesn't care whether the CLD has been added to
>>>> the graph.
>>>> Your instead code requires a comment that replace_if_null is really
>>>> a compare exchange and has an extra read of the original value, so
>>>> I am leaving what I have which is clearer to me.
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/classfile/verifier.cpp
>>>>> 71 static void* verify_byte_codes_fn() {
>>>>> 72 if (OrderAccess::load_acquire(&_verify_byte_codes_fn) ==
>>>>> NULL) {
>>>>> 73 void *lib_handle = os::native_java_library();
>>>>> 74 void *func = os::dll_lookup(lib_handle,
>>>>> "VerifyClassCodesForMajorVersion");
>>>>> 75 OrderAccess::release_store(&_verify_byte_codes_fn, func);
>>>>> 76 if (func == NULL) {
>>>>> 77 _is_new_verify_byte_codes_fn = false;
>>>>> 78 func = os::dll_lookup(lib_handle, "VerifyClassCodes");
>>>>> 79 OrderAccess::release_store(&_verify_byte_codes_fn, func);
>>>>> 80 }
>>>>> 81 }
>>>>> 82 return (void*)_verify_byte_codes_fn;
>>>>> 83 }
>>>>>
>>>>> [pre-existing]
>>>>>
>>>>> I think this code has race problems; a caller could unexpectedly and
>>>>> inappropriately return NULL. Consider the case where there is no
>>>>> VerifyClassCodesForMajorVersion, but there is VerifyClassCodes.
>>>>>
>>>>> The variable is initially NULL.
>>>>>
>>>>> Both Thread1 and Thread2 reach line 73, having both seen a NULL value
>>>>> for the variable.
>>>>>
>>>>> Thread1 reaches line 80, setting the variable to VerifyClassCodes.
>>>>>
>>>>> Thread2 reaches line 76, resetting the variable to NULL.
>>>>>
>>>>> Thread1 reads the now (momentarily) NULL value and returns it.
>>>>>
>>>>> I think the first release_store should be conditional on func !=
>>>>> NULL.
>>>>> Also, the usage of _is_new_verify_byte_codes_fn seems suspect.
>>>>> And a minor additional nit: the cast in the return is unnecessary.
>>>>
>>>> Yes, this looks like a bug. I'll cut/paste this and file it. It
>>>> may be that this is support for the old verifier in old jdk
>>>> versions that can be cleaned up.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/code/nmethod.cpp
>>>>> 1664 nmethod* observed_mark_link = _oops_do_mark_link;
>>>>> 1665 if (observed_mark_link == NULL) {
>>>>> 1666 // Claim this nmethod for this thread to mark.
>>>>> 1667 if (Atomic::cmpxchg_if_null(NMETHOD_SENTINEL,
>>>>> &_oops_do_mark_link)) {
>>>>>
>>>>> With these changes, the only use of observed_mark_link is in the if.
>>>>> I'm not sure that variable is really useful anymore, e.g. just use
>>>>>
>>>>> if (_oops_do_mark_link == NULL) {
>>>>
>>>> Ok fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
>>>>>
>>>>> In CMSCollector::par_take_from_overflow_list, if BUSY and prefix were
>>>>> of type oopDesc*, I think there would be a whole lot fewer casts and
>>>>> cast_to_oop's. Later on, I think suffix_head,
>>>>> observed_overflow_list,
>>>>> and curr_overflow_list could also be oopDesc* instead of oop to
>>>>> eliminate more casts.
>>>>
>>>> I actually tried to make this change but ran into more fan out that
>>>> way, so went back and just fixed the cmpxchg calls to cast oops to
>>>> oopDesc* and things were less perturbed that way.
>>>>>
>>>>> And some similar changes in CMSCollector::par_push_on_overflow_list.
>>>>>
>>>>> And similarly in parNewGeneration.cpp, in push_on_overflow_list and
>>>>> take_from_overflow_list_work.
>>>>>
>>>>> As noted in the comments for JDK-8165857, the lists and "objects"
>>>>> involved here aren't really oops, but rather the shattered remains of
>>>>
>>>> Yes, somewhat horrified at the value of BUSY.
>>>>> oops. The suggestion there was to use HeapWord* and carry through
>>>>> the
>>>>> fanout; what was actually done was to change _overflow_list to
>>>>> oopDesc* to minimize fanout, even though that's kind of lying to the
>>>>> type system. Now, with the cleanup of cmpxchg_ptr and such, we're
>>>>> paying the price of doing the minimal thing back then.
>>>>
>>>> I will file an RFE about cleaning this up. I think what I've done
>>>> was the minimal thing.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
>>>>> 7960 Atomic::add(-n, &_num_par_pushes);
>>>>>
>>>>> Atomic::sub
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/cms/parNewGeneration.cpp
>>>>> 1455 Atomic::add(-n, &_num_par_pushes);
>>>> fixed.
>>>>> Atomic::sub
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/dirtyCardQueue.cpp
>>>>> 283 void* actual = Atomic::cmpxchg(next,
>>>>> &_cur_par_buffer_node, nd);
>>>>> ...
>>>>> 289 nd = static_cast<BufferNode*>(actual);
>>>>>
>>>>> Change actual's type to BufferNode* and remove the cast on line 289.
>>>>
>>>> fixed. missed that one. gross.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>>>>>
>>>>> [pre-existing]
>>>>> 3499 old = (CompiledMethod*)_postponed_list;
>>>>>
>>>>> I think that cast is only needed because
>>>>> G1CodeCacheUnloadingTask::_postponed_list is incorrectly typed as
>>>>> "volatile CompiledMethod*", when I think it ought to be
>>>>> "CompiledMethod* volatile".
>>>>>
>>>>> I think G1CodeCacheUnloading::_claimed_nmethod is similarly
>>>>> mis-typed,
>>>>> with a similar should not be needed cast:
>>>>> 3530 first = (CompiledMethod*)_claimed_nmethod;
>>>>>
>>>>> and another for _postponed_list here:
>>>>> 3552 claim = (CompiledMethod*)_postponed_list;
>>>>
>>>> I've fixed this. C++ is so confusing about where to put the
>>>> volatile. Everyone has been tripped up by it.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/g1HotCardCache.cpp
>>>>> 77 jbyte* previous_ptr = (jbyte*)Atomic::cmpxchg(card_ptr,
>>>>>
>>>>> I think the cast of the cmpxchg result is no longer needed.
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/g1PageBasedVirtualSpace.cpp
>>>>> 254 char* touch_addr =
>>>>> (char*)Atomic::add(actual_chunk_size, &_cur_addr) -
>>>>> actual_chunk_size;
>>>>>
>>>>> I think the cast of the add result is no longer needed.
>>>> got it already.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/g1StringDedup.cpp
>>>>> 213 return (size_t)Atomic::add(partition_size, &_next_bucket)
>>>>> - partition_size;
>>>>>
>>>>> I think the cast of the add result is no longer needed.
>>>>
>>>> I was slacking in the g1 files. fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/heapRegionRemSet.cpp
>>>>> 200 PerRegionTable* res =
>>>>> 201 Atomic::cmpxchg(nxt, &_free_list, fl);
>>>>>
>>>>> Please remove the line break, now that the code has been simplified.
>>>>>
>>>>> But wait, doesn't this alloc exhibit classic ABA problems? I *think*
>>>>> this works because alloc and bulk_free are called in different
>>>>> phases,
>>>>> never overlapping.
>>>>
>>>> I don't know. Do you want to file a bug to investigate this?
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>>>> 295 SparsePRT* res =
>>>>> 296 Atomic::cmpxchg(sprt, &_head_expanded_list, hd);
>>>>> and
>>>>> 307 SparsePRT* res =
>>>>> 308 Atomic::cmpxchg(next, &_head_expanded_list, hd);
>>>>>
>>>>> I'd rather not have the line breaks in these either.
>>>>>
>>>>> And get_from_expanded_list also appears to have classic ABA problems.
>>>>> I *think* this works because add_to_expanded_list and
>>>>> get_from_expanded_list are called in different phases, never
>>>>> overlapping.
>>>>
>>>> Fixed, same question as above? Or one bug to investigate both?
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/gc/shared/taskqueue.inline.hpp
>>>>> 262 return (size_t) Atomic::cmpxchg((intptr_t)new_age._data,
>>>>> 263 (volatile intptr_t *)&_data,
>>>>> 264 (intptr_t)old_age._data);
>>>>>
>>>>> This should be
>>>>>
>>>>> return Atomic::cmpxchg(new_age._data, &_data, old_age._data);
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/interpreter/bytecodeInterpreter.cpp
>>>>> This doesn't have any casts, which I think is correct.
>>>>> 708 if (Atomic::cmpxchg(header, rcvr->mark_addr(),
>>>>> mark) == mark) {
>>>>>
>>>>> but these do.
>>>>> 718 if (Atomic::cmpxchg((void*)new_header,
>>>>> rcvr->mark_addr(), mark) == mark) {
>>>>> 737 if (Atomic::cmpxchg((void*)new_header,
>>>>> rcvr->mark_addr(), header) == header) {
>>>>>
>>>>> I'm not sure how the ones with casts even compile? mark_addr() seems
>>>>> to be a markOop*, which is a markOopDesc**, where markOopDesc is a
>>>>> class. void* is not implicitly convertible to markOopDesc*.
>>>>>
>>>>> Hm, this entire file is #ifdef CC_INTERP. Is this zero-only
>>>>> code? Or
>>>>> something like that?
>>>>>
>>>>> Similarly here:
>>>>> 906 if (Atomic::cmpxchg(header, lockee->mark_addr(),
>>>>> mark) == mark) {
>>>>> and
>>>>> 917 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), mark) == mark) {
>>>>> 935 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), header) == header) {
>>>>>
>>>>> and here:
>>>>> 1847 if (Atomic::cmpxchg(header,
>>>>> lockee->mark_addr(), mark) == mark) {
>>>>> 1858 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), mark) == mark) {
>>>>> 1878 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), header) == header) {
>>>>>
>>>>> and here:
>>>>> 1847 if (Atomic::cmpxchg(header,
>>>>> lockee->mark_addr(), mark) == mark) {
>>>>> 1858 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), mark) == mark) {
>>>>> 1878 if (Atomic::cmpxchg((void*)new_header,
>>>>> lockee->mark_addr(), header) == header) {
>>>>
>>>> I've changed all these. This is part of Zero.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/memory/metaspace.cpp
>>>>> 1502 size_t value = OrderAccess::load_acquire(&_capacity_until_GC);
>>>>> ...
>>>>> 1537 return (size_t)Atomic::sub((intptr_t)v, &_capacity_until_GC);
>>>>>
>>>>> These and other uses of _capacity_until_GC suggest that variable's
>>>>> type should be size_t rather than intptr_t. Note that I haven't done
>>>>> a careful check of uses to see if there are any places where such a
>>>>> change would cause problems.
>>>>
>>>> Yes, I had a hard time with metaspace.cpp because I agree
>>>> _capacity_until_GC should be size_t. Tried to make this change
>>>> and it cascaded a bit. I'll file an RFE to change this type
>>>> separately.
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/constantPool.cpp
>>>>> 229 OrderAccess::release_store((Klass* volatile *)adr, k);
>>>>> 246 OrderAccess::release_store((Klass* volatile *)adr, k);
>>>>> 514 OrderAccess::release_store((Klass* volatile *)adr, k);
>>>>>
>>>>> Casts are not needed.
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/constantPool.hpp
>>>>> 148 volatile intptr_t adr =
>>>>> OrderAccess::load_acquire(obj_at_addr_raw(which));
>>>>>
>>>>> [pre-existing]
>>>>> Why is adr declared volatile?
>>>>
>>>> golly beats me. concurrency is scary, especially in the constant
>>>> pool.
>>>> The load_acquire() should make sure the value is fetched from
>>>> memory so volatile is unneeded.
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/cpCache.cpp
>>>>> 157 intx newflags = (value & parameter_size_mask);
>>>>> 158 Atomic::cmpxchg(newflags, &_flags, (intx)0);
>>>>>
>>>>> This is a nice demonstration of why I wanted to include some value
>>>>> preserving integral conversions in cmpxchg, rather than requiring
>>>>> exact type matching in the integral case. There have been some
>>>>> others
>>>>> that I haven't commented on. Apparently we (I) got away with
>>>>> including such conversions in Atomic::add, which I'd forgotten about.
>>>>> And see comment regarding Atomic::sub below.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/cpCache.hpp
>>>>> 139 volatile Metadata* _f1; // entry specific metadata
>>>>> field
>>>>>
>>>>> [pre-existing]
>>>>> I suspect the type should be Metadata* volatile. And that would
>>>>> eliminate the need for the cast here:
>>>>>
>>>>> 339 Metadata* f1_ord() const { return
>>>>> (Metadata *)OrderAccess::load_acquire(&_f1); }
>>>>>
>>>>> I don't know if there are any other changes needed or desirable
>>>>> around
>>>>> _f1 usage.
>>>>
>>>> yes, fixed this.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/method.hpp
>>>>> 139 volatile address from_compiled_entry() const { return
>>>>> OrderAccess::load_acquire(&_from_compiled_entry); }
>>>>> 140 volatile address from_compiled_entry_no_trampoline() const;
>>>>> 141 volatile address from_interpreted_entry() const{ return
>>>>> OrderAccess::load_acquire(&_from_interpreted_entry); }
>>>>>
>>>>> [pre-existing]
>>>>> The volatile qualifiers here seem suspect to me.
>>>>
>>>> Again much suspicion about concurrency and giant pain, which I
>>>> remember, of debugging these when they were broken.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/oops/oop.inline.hpp
>>>>> 391 narrowOop old = (narrowOop)Atomic::xchg(val,
>>>>> (narrowOop*)dest);
>>>>>
>>>>> Cast of return type is not needed.
>>>>
>>>> fixed.
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/prims/jni.cpp
>>>>>
>>>>> [pre-existing]
>>>>>
>>>>> copy_jni_function_table should be using Copy::disjoint_words_atomic.
>>>>
>>>> yuck.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/prims/jni.cpp
>>>>>
>>>>> [pre-existing]
>>>>>
>>>>> 3892 // We're about to use Atomic::xchg for synchronization.
>>>>> Some Zero
>>>>> 3893 // platforms use the GCC builtin __sync_lock_test_and_set
>>>>> for this,
>>>>> 3894 // but __sync_lock_test_and_set is not guaranteed to do
>>>>> what we want
>>>>> 3895 // on all architectures. So we check it works before
>>>>> relying on it.
>>>>> 3896 #if defined(ZERO) && defined(ASSERT)
>>>>> 3897 {
>>>>> 3898 jint a = 0xcafebabe;
>>>>> 3899 jint b = Atomic::xchg(0xdeadbeef, &a);
>>>>> 3900 void *c = &a;
>>>>> 3901 void *d = Atomic::xchg(&b, &c);
>>>>> 3902 assert(a == (jint) 0xdeadbeef && b == (jint) 0xcafebabe,
>>>>> "Atomic::xchg() works");
>>>>> 3903 assert(c == &b && d == &a, "Atomic::xchg() works");
>>>>> 3904 }
>>>>> 3905 #endif // ZERO && ASSERT
>>>>>
>>>>> It seems rather strange to be testing Atomic::xchg() here, rather
>>>>> than
>>>>> as part of unit testing Atomic? Fail unit testing => don't try to
>>>>> use...
>>>>
>>>> This is zero. I'm not touching this.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/prims/jvmtiRawMonitor.cpp
>>>>> 130 if (Atomic::cmpxchg_if_null((void*)Self, &_owner)) {
>>>>> 142 if (_owner == NULL &&
>>>>> Atomic::cmpxchg_if_null((void*)Self, &_owner)) {
>>>>>
>>>>> I think these casts aren't needed. _owner is void*, and Self is
>>>>> Thread*, which is implicitly convertible to void*.
>>>>>
>>>>> Similarly here, for the THREAD argument:
>>>>> 280 Contended = Atomic::cmpxchg((void*)THREAD, &_owner,
>>>>> (void*)NULL);
>>>>> 283 Contended = Atomic::cmpxchg((void*)THREAD, &_owner,
>>>>> (void*)NULL);
>>>>
>>>> Okay, let me see if the compiler(s) eat that. (yes they do)
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/prims/jvmtiRawMonitor.hpp
>>>>>
>>>>> This file is in the webrev, but seems to be unchanged.
>>>>
>>>> It'll be cleaned up with the the commit and not be part of the
>>>> changeset.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/atomic.hpp
>>>>> 520 template<typename I, typename D>
>>>>> 521 inline D Atomic::sub(I sub_value, D volatile* dest) {
>>>>> 522 STATIC_ASSERT(IsPointer<D>::value || IsIntegral<D>::value);
>>>>> 523 // Assumes two's complement integer representation.
>>>>> 524 #pragma warning(suppress: 4146)
>>>>> 525 return Atomic::add(-sub_value, dest);
>>>>> 526 }
>>>>>
>>>>> I'm pretty sure this implementation is incorrect. I think it
>>>>> produces
>>>>> the wrong result when I and D are both unsigned integer types and
>>>>> sizeof(I) < sizeof(D).
>>>>
>>>> Can you suggest a correction? I just copied Atomic::dec().
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>> 304 intptr_t v = Atomic::cmpxchg((intptr_t)_LBIT,
>>>>> &_LockWord.FullWord, (intptr_t)0); // agro ...
>>>>>
>>>>> _LBIT should probably be intptr_t, rather than an enum. Note that the
>>>>> enum type is unused. The old value here is another place where an
>>>>> implicit widening of same signedness would have been nice. (Such
>>>>> implicit widening doesn't work for enums, since it's unspecified
>>>>> whether they default to signed or unsigned representation, and
>>>>> implementatinos differ.)
>>>>
>>>> This would be a good/simple cleanup. I changed it to const
>>>> intptr_t _LBIT = 1;
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/mutex.hpp
>>>>>
>>>>> [pre-existing]
>>>>>
>>>>> I think the Address member of the SplitWord union is unused. Looking
>>>>> at AcquireOrPush (and others), I'm wondering whether it *should* be
>>>>> used there, or whether just using intptr_t casts and doing integral
>>>>> arithmetic (as is presently being done) is easier and clearer.
>>>>>
>>>>> Also the _LSBINDEX macro probably ought to be defined in mutex.cpp
>>>>> rather than polluting the global namespace. And technically, that
>>>>> name is reserved word.
>>>>
>>>> I moved both this and _LBIT into the top of mutex.cpp since they
>>>> are used there.
>>>> Cant define const intptr_t _LBIT =1; in a class in our version of C++.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/objectMonitor.cpp
>>>>> 252 void * cur = Atomic::cmpxchg((void*)Self, &_owner,
>>>>> (void*)NULL);
>>>>> 409 if (Atomic::cmpxchg_if_null((void*)Self, &_owner)) {
>>>>> 1983 ox = (Thread*)Atomic::cmpxchg((void*)Self, &_owner,
>>>>> (void*)NULL);
>>>>>
>>>>> I think the casts of Self aren't needed.
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/objectMonitor.cpp
>>>>> 995 if (!Atomic::cmpxchg_if_null((void*)THREAD, &_owner)) {
>>>>> 1020 if (!Atomic::cmpxchg_if_null((void*)THREAD, &_owner)) {
>>>>>
>>>>> I think the casts of THREAD aren't needed.
>>>>
>>>> nope, fixed.
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/objectMonitor.hpp
>>>>> 254 markOopDesc* volatile* header_addr();
>>>>>
>>>>> Why isn't this volatile markOop* ?
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/synchronizer.cpp
>>>>> 242 Atomic::cmpxchg_if_null((void*)Self, &(m->_owner))) {
>>>>>
>>>>> I think the cast of Self isn't needed.
>>>>
>>>> fixed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/synchronizer.cpp
>>>>> 992 for (; block != NULL; block = (PaddedEnd<ObjectMonitor>
>>>>> *)next(block)) {
>>>>> 1734 for (; block != NULL; block = (PaddedEnd<ObjectMonitor>
>>>>> *)next(block)) {
>>>>>
>>>>> [pre-existing]
>>>>> All calls to next() pass a PaddedEnd<ObjectMonitor>* and cast the
>>>>> result. How about moving all that behavior into next().
>>>>
>>>> I fixed this next() function, but it necessitated a cast to
>>>> FreeNext field. The PaddedEnd<> type was intentionally not
>>>> propagated to all the things that use it. Which is a shame
>>>> because there are a lot more casts to PaddedEnd<ObjectMonitor> that
>>>> could have been removed.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/synchronizer.cpp
>>>>> 1970 if (monitor > (ObjectMonitor *)&block[0] &&
>>>>> 1971 monitor < (ObjectMonitor *)&block[_BLOCKSIZE]) {
>>>>>
>>>>> [pre-existing]
>>>>> Are the casts needed here? I think PaddedEnd<ObjectMonitor> is
>>>>> derived from ObjectMonitor, so implicit conversions should apply.
>>>>
>>>> prob not. removed them.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/synchronizer.hpp
>>>>> 28 #include "memory/padded.hpp"
>>>>> 163 static PaddedEnd<ObjectMonitor> * volatile gBlockList;
>>>>>
>>>>> I was going to suggest as an alternative just making gBlockList a
>>>>> file
>>>>> scoped variable in synchronizer.cpp, since it isn't used outside of
>>>>> that file. Except that it is referenced by vmStructs. Curses!
>>>>
>>>> It's also used by the SA.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/runtime/thread.cpp
>>>>> 4707 intptr_t w = Atomic::cmpxchg((intptr_t)LOCKBIT, Lock,
>>>>> (intptr_t)0);
>>>>>
>>>>> This and other places suggest LOCKBIT should be defined as intptr_t,
>>>>> rather than as an enum value. The MuxBits enum type is unused.
>>>>>
>>>>> And the cast of 0 is another case where implicit widening would be
>>>>> nice.
>>>>
>>>> Making LOCKBIT a const intptr_t = 1 removes a lot of casts.
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>> src/hotspot/share/services/mallocSiteTable.cpp
>>>>> 261 bool MallocSiteHashtableEntry::atomic_insert(const
>>>>> MallocSiteHashtableEntry* entry) {
>>>>> 262 return Atomic::cmpxchg_if_null(entry, (const
>>>>> MallocSiteHashtableEntry**)&_next);
>>>>> 263 }
>>>>>
>>>>> I think the problem here that is leading to the cast is that
>>>>> atomic_insert is taking a const T*. Note that it's only caller
>>>>> passes
>>>>> a non-const T*.
>>>>
>>>> I'll change the type to non-const. We try to use consts...
>>>>
>>>> Thanks for the detailed review! The gcc compiler seems happy so
>>>> far, I'll post a webrev of the result of these changes after fixing
>>>> Atomic::sub() and seeing how the other compilers deal with these
>>>> changes.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>>
>>>>>
>>>>
>>>
More information about the hotspot-dev
mailing list