RFR(S): 8152358 - code and comment cleanups found during the hunt for 8077392
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Mar 29 23:30:11 UTC 2016
Hi Carsten!
Thanks for taking the time to jump into this review.
Responses embedded below...
On 3/29/16 9:56 AM, Carsten Varming wrote:
> Dear Dan,
>
> Generally a very nice clean-up, but I have a few questions.
>
> Quick question: Is it time to remove all those unused perf
> variables: _sync_EmptyNotifications, _sync_Notifications, _sync_SlowEnter, _sync_SlowExit, _sync_SlowNotify, _sync_SlowNotifyAll, _sync_FailedSpins, _sync_SuccessfulSpins, _sync_PrivateA, _sync_PrivateB, _sync_MonInCirculation,
> and _sync_MonScavenged.
Claes chimed in on the thread with a mention of:
> there's already an RFE for that:
https://bugs.openjdk.java.net/browse/JDK-8068593
so I think the above comment is covered.
> synchronizer.cpp:290 you mention that the monitor is either being
> inflated or has already been inflated, but mark->has_monitor() returns
> false if the monitor is being inflated, and this fact is important for
> the assert in line 292 as it would otherwise dereference a NULL pointer.
You are correct. This line:
L290: // Is inflating or inflated.
is wrong and I need to delete the "inflating or" part, but then...
> BTW., should the if-clause, down to above the return, for the
> recursive case in fast_exit be removed in the product build? It looks
> like a few dead loads of volatile values to me.
Looking at the entire block:
281 if (dhw == NULL) {
282 // Marked as a recursive stack-lock.
283 // Diagnostics -- Could be: stack-locked, inflating, inflated.
284 mark = object->mark();
285 assert(!mark->is_neutral(), "invariant");
286 if (mark->has_locker() && mark != markOopDesc::INFLATING()) {
287 assert(THREAD->is_lock_owned((address)mark->locker()), "invariant");
288 }
289 if (mark->has_monitor()) {
290 // Is inflating or inflated.
291 ObjectMonitor * m = mark->monitor();
292 assert(((oop)(m->object()))->mark() == mark, "invariant");
293 assert(m->is_entered(THREAD), "invariant");
294 }
295 return;
296 }
It is a bit of a mess in many ways...
If this line:
L284: mark = object->mark();
happens to return a 'mark' value of markOopDesc::INFLATING(),
then 'mark' is NULL and these lines all blow up:
L285: assert(!mark->is_neutral(), "invariant");
L286: if (mark->has_locker() && mark != markOopDesc::INFLATING()) {
L287: assert(THREAD->is_lock_owned((address)mark->locker()),
"invariant");
L289: if (mark->has_monitor()) {
L291: ObjectMonitor * m = mark->monitor();
In fact, the first assert():
L277: assert(!object->mark()->has_bias_pattern(), "should not see
bias pattern here");
is a problem if 'mark()' happens to return markOopDesc::INFLATING().
I'm going to have to take another look at all this sanity
checking code and make it a little more sane. :-)
> There is a comment in synchronizer.cpp:1419 that mentions that it is
> safe to read the displaced mark word of a lock as the thread exiting
> the lock will wait for the object mark word to be different from
> INFLATING. I don't see fast_exit waiting for the mark word to be
> different from INFLATING in the recursive case, so could the
> mark->displaced_mark_helper() in line 1423 be reading an arbitrary
> value on the stack of the thread holding the lock? Is this a bug?
You followed up on the above comment with your own reply:
> I think I figured out why what the comment " The owner can't die or
> unwind past the lock while our INFLATING object is in the mark" means.
> The displaced mark word is in the first lock on the stack of the
> thread owning the lock. That mean the only case to worry about the the
> non-recursive exit which is handled by a call to inflate as INFLATING
> is different from the pointer to the lock. It would be nice with a
> comment explaining why the recursive exit does not need to wait, i.e.,
> the object mark word always points to the first lock on the stack of
> the thread currently owning the lock.
With your reply above I think you've reached the conclusion that the
code is working, but we need better comments. I think the whole
INFLATING mechanism needs to be looked at for better comments which
I wasn't trying to do with this fix.
You ended that reply with:
> Anyway, the clean-up looks good.
but I'm thinking that you meant that for just the one comment thread
in your original post and not for your entire review.
> I don't understand the comment "Is recursive stack-lock" in
> synchronizer.cpp:303. The recursive case was handled above and ended
> with a return. This should be the non-recursive case.
Agreed. I think I screwed up that comment when I did my extraction
of the cleanup changes for this patch. I will delete that line.
> I also don't understand how it can be safe to install the old
> displaced mark word read at the top of the method as a different
> thread could have installed a hashCode into the displaced mark word
> between the read and the CAS into the object mark word. Could this gap
> drop the hashCode, i.e., the value of the java method void
> Object.hashCode() could change for the object?
You followed up on the above comment with your own reply:
> Re my last comment about a potential race between FastHashCode and
> fast_exit. Never mind, I just noticed that FastHashCode will inflate
> the lock to monitor before installing a hash code.
so I think the above comment is covered.
Dan
>
> Carsten
>
> On Mon, Mar 28, 2016 at 7:19 PM, Daniel D. Daugherty
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> wrote:
>
> On 3/24/16 3:59 PM, Daniel D. Daugherty wrote:
>
> On 3/24/16 3:27 PM, Vladimir Kozlov wrote:
>
> In sharedRuntime.cpp you could use BasicObjectLock::size()
> as you did in c1_LIRAssembler_x86.cpp
>
>
> I'll take a look. I may leave that cleanup to another pass, if
> you would be OK with it... I'm nearing the end of my 72 hour
> stress test cycle and don't want to repeat it... :-)
>
>
> Here's the C1 change:
>
> $ hg diff src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
> diff -r b9efb94d011a src/cpu/x86/vm/c1_LIRAssembler_x86.cpp
> --- a/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp Mon Mar 07
> 11:28:06 2016 -0800
> +++ b/src/cpu/x86/vm/c1_LIRAssembler_x86.cpp Mon Mar 28
> 16:12:30 2016 -0700
> @@ -312,7 +312,7 @@ void LIR_Assembler::osr_entry() {
> Register OSR_buf = osrBufferPointer()->as_pointer_register();
> { assert(frame::interpreter_frame_monitor_size() ==
> BasicObjectLock::size(), "adjust code below");
> int monitor_offset = BytesPerWord * method()->max_locals() +
> - (2 * BytesPerWord) * (number_of_locks - 1);
> + (BasicObjectLock::size() * BytesPerWord) * (number_of_locks
> - 1);
> // SharedRuntime::OSR_migration_begin() packs BasicObjectLocks in
> // the OSR buffer using 2 word entries: first the lock and then
> // the oop.
>
>
> Here's the existing code in SharedRuntime::OSR_migration_begin():
>
> // Allocate temp buffer, 1 word per local & 2 per active monitor
> int buf_size_words = max_locals + active_monitor_count *
> (sizeof(BasicObjectLock) / wordSize);
> intptr_t *buf = NEW_C_HEAP_ARRAY(intptr_t,buf_size_words, mtCode);
>
>
> so is your suggestion to change:
>
> sizeof(BasicObjectLock) / wordSize
>
> to:
>
> BasicObjectLock::size()
>
> or am I misunderstanding what you mean?
>
> Dan
>
>
More information about the hotspot-runtime-dev
mailing list