Hi Dan,
On 8/28/2015 2:12 PM, Daniel D. Daugherty wrote:
. . .
Not easily in ReenterI(), the OM_PERFDATA_OP(FutileWakeups, inc()) call
is at the bottom of the for-loop and the OrderAccess::fence() call at
the end of the function is outside the loop. This would result in lost
FutileWakeups increments.
Yes, you'd have to keep a local count, and then add the total outside
the loop for both Enter/Reenter, after the fence. But I see what you
mean about the other exit paths in Enter. (The more I look at this
code, the more I remember it...
I'm sure that is a wonderfully loving memory too!
Absolutely! 8^)
BTW, are those knob_ setting defaults ever going to be moved to a
platform specific-module? That was my beef (well, one of them) in 2
different ports. Or is the goal to make monitors so well self-tuning
that they can go away? Sorry for the digression... 8^))
Dice is working on another idea to move tuning to a separate loadable
module which is why we deferred the "adaptive spin" and "SpinPause on
SPARC" buckets for the Contended Locking project.
At any rate, as you say, perhaps it's not worth it to leverage the
fences, though it could be done.
OK so we're agreed on no change here.
So in ReenterI() the OM_PERFDATA_OP(FutileWakeups, inc()) call
immediately
follows an OrderAccess::fence() call. Doesn't that make that
increment as
"safe" as it can be without having a real lock?
You've got a release() (and and short nap!) with the store in
PerfDataManager::destroy() to try to close the window somewhat.
Yes, I modeled that after:
src/share/vm/runtime/perfMemory.cpp:
83 void PerfMemory::initialize() {
<snip>
156 OrderAccess::release_store(&_initialized, 1);
157 }
But I think rather than the release_store() you used, you want a
store, followed by a release(). release_store() puts a fence
before the store to ensure earlier updates are seen before the
current one, no?
Yup, and I see I got my reasoning wrong. The code I modeled
is right because you want to flush all the inits and it's OK
if the _initialized transition from '0' -> '1' is lazily seen.
For my shutdown use, we are transitioning from '1' -> '0' and
we need that to be seen proactively so:
OrderAccess::release_store(&_has_PerfData, 0);
OrderAccess::storeload();
which is modeled after _owner field transitions from non-zero
-> NULL in ObjectMonitor.cpp
It's not clear to me why the store needs to be a release_store in
this case, as long as the storeload() follows it. You're not
protecting any earlier writes. ?
I'm following the model that Dice uses for ObjectMonitors when we
change the _owner field from non-NULL -> NULL. There are some long
comments in the ObjectMonitor.cpp stuff that talk about why it is
done this way. I'm planning to file a cleanup bug for the non-NULL
-> NULL transition stuff because the comments and code are not
consistent.
But that's in lock exit, so the release is needed to ensure all
outstanding writes are seen before the owner is set to null. There's
nothing analogous in this case.
However, since this will be executed once per JVM shutdown, having an
extra release() isn't a big deal...
Tom
