Comments inline ...
On 6/1/18, 7:45 AM, Gary Adams wrote:
PS.
PerfCounter.java
lb field should be left as final. You can work around
definite-assignment problems by using a local variable and only
assigning to the field after the catch block:
private PerfCounter(String name, int type) {
this.name = name;
+ LongBuffer temp = null;
+ try {
ByteBuffer bb = perf.createLong(name, type, U_None,
0L);
bb.order(ByteOrder.nativeOrder());
+ temp = bb.asLongBuffer();
+ } catch (IOException e) {
+ // may not be available
+ }
+ this.lb = temp;
}
Thanks for the tip. The PerfCounter changes were the last thing I
dropped in
after adding the IOException and fixing the places where things did
not compile.
Fundamentally, what sort of things will break, when the previous
interface
did not expose the possibility that the perf memory was not initialized
as assumed in the earlier code. Does the upper level code "have to"
be aware, or is the contract to do a "best effort" if something lower
down
is not as expected.
Hi Gary,
Meta-question partially raised in the the recent bug report
comments: do
we care about this any more? Does anything need to disable UsePerfData?
Can we not deprecate it in 11 then obsolete in 12? That would be a lot
simpler.
My own bias is that many of the motivations for faster startup and
smaller footprint
are still useful goals. If not for embedded systems, then for
container environments.
A process started with -XX:-UsePerfData does not support external attach
and does not maintain the internal variables for the self attach.
That aside ...
It's hard to see how all the different paths in these related API's
stitch together - especially when you have to consider the API being
used on the current VM or a different one. I'm not at all clear what
the
control flow is, so it is hard to see exactly how the initialization
issue is being handled. As I wrote in the initial report it is unclear
exactly which parts of the "perf" subsystem are impacted by the
value of
UsePerfData.
Starting with the Perf.java class initialization, I added the
exposure of
the PerfMemory::is_initialized().
Perf.java:
559 private native boolean isInitialized();
560
561 private static native void registerNatives();
562
563 static {
564 registerNatives();
565 instance = new Perf();
566 initialized = instance.isInitialized();
567 }
perf.cpp:
298 PERF_ENTRY(jboolean, Perf_IsInitialized(JNIEnv *env, jobject perf))
299
300 PerfWrapper("Perf_IsInitialized");
301
302 return PerfMemory::is_initialized();
303
304 PERF_END
From the VM perspective UsePerfData is both a command line argument
and a flag to be used whenever that information is needed. There are
scattered checks to deal with setup and shutdown, but also the runtime
fulfillment of the PerfMemory data structures.
Looking at perfMemory_init() and perfMemory_exit(), I concluded that
the UsePerfData flag essentially noops all of the perfMemory
allocation and
updating. The only information required in the upper layers of the
Perf.java
code is the fact that the perf memory is not initialized.
perfMemory.cpp:
58 void perfMemory_init() {
59
60 if (!UsePerfData) return;
61
62 PerfMemory::initialize();
63 }
64
65 void perfMemory_exit() {
66
67 if (!UsePerfData) return;
68 if (!PerfMemory::is_usable()) return;
69
...
90 void PerfMemory::initialize() {
...
159 OrderAccess::release_store(&_initialized, 1);
160 }
...
271 bool PerfMemory::is_initialized() {
272 return OrderAccess::load_acquire(&_initialized) != 0;
273 }
Can you summarize the role of the is_initialized check versus a direct
UsePerfData check on the VM side i.e. which should be checked when? I
immediately see an issue that Perf_Detach is a no-op when !UsePerfdata,
yet Perf_Attach doesn't examine the flag at all! But perhaps we never
reach those methods because of a higher-level call filtering it out ??
There is split path when attaching to self versus attaching to other
running VM processes. The extra detach processing is only needed
for the remote VM case. If attach fails, there is no need to detach.
Perf.java:
...
274 private ByteBuffer attachImpl(String user, int lvmid, int
mode)
275 throws IllegalArgumentException, IOException
276 {
277 final ByteBuffer b = attach(user, lvmid, mode);
278
279 if (lvmid == 0) {
280 // The native instrumentation buffer for this Java
virtual
281 // machine is never unmapped.
282 return b;
283 }
284 else {
285 // This is an instrumentation buffer for another
Java virtual
286 // machine with native resources that need to be
managed. We
287 // create a duplicate of the native ByteBuffer and
manage it
288 // with a Cleaner. When the duplicate becomes
phantom reachable,
289 // the native resources will be released.
290
291 final ByteBuffer dup = b.duplicate();
292
293 CleanerFactory.cleaner()
294 .register(dup, new
CleanerAction(instance, b));
295 return dup;
296 }
297 }
More below ...
David
On 1/06/2018 5:09 AM, Gary Adams wrote:
>/A patch was done previously to prevent an error when
-XX:-UsePerfData />/is used, but this bug was filed to make the
setting more visible in the />/jdk.internal.perf package. />//>/
Webrev: http://cr.openjdk.java.net/~gadams/8069149/webrev.00/
<http://cr.openjdk.java.net/%7Egadams/8069149/webrev.00/> />/
Issue: https://bugs.openjdk.java.net/browse/JDK-8069149 />//>/I have
tried a few initial tests using: />//>/ make run-tests \ />/
TEST_OPTS=VM_OPTIONS=-XX:-UsePerfData \ />/
TEST=open/test/jdk/sun/management/jmxremote />//>/While I'm tracking
down one test timeout, I'd like to get some />/feedback on the
approach used here : />//>/ - the basic mechanism propagates the
"is initialized" state up to />/Perf.java; />/ does the state
need to be exposed to users getPerf(), or is it />/sufficient/
Within PerfMemory UsePerfData and is_usable() are used for guarding
execution. Perhaps that is what needs to be propagated up?
/The is_usable() state is used to ensure the memory is initialized
and not yet detached. We don't need that specific condition here. >
/Unclear what the control flow is here. I don't see that Perf triggers
initialization of the perf subsystem, so what guarantees
initialization should have happened by the time the Perf class is
initialized ??
The private constructor and the static initializer in Perf ensures
the singleton behavior at class initialization. The perfMemory_init()
is handled during VM initialization.
init.cpp:
90 void vm_init_globals() {
91 check_ThreadShadow();
92 basic_types_init();
93 eventlog_init();
94 mutex_init();
95 chunkpool_init();
96 perfMemory_init();
97 SuspendibleThreadSet_init();
98 }
>/ - an existing use of IOException was used in the case of
attach /> >/failures; /> >/ added IOExceptions to the
createXXX methods if the memory was not /> >/initialized /
On the VM side can you at least add a message to the IOException so
that the user can tell why it was thrown.
Initially, I included a message with the IOException, and noticed the
other
exceptions being thrown in the surrounding code did not include a
message,
so opted for the empty message. There is no ambiguity here.
>/ - presuming a CSR would be needed to cover the IOExceptions /
It's not a public API so no CSR should be needed.
Good to know.
>/ - it appears that jdk.internal.perf has very limited usage, />
>/ is it still needed/used (?) /
I can't answer that part, but I don't think UsePerfData is needed
any more.
If this turns into "removal of UsePerfData" support, we should move
the bug
to the runtime group. I grabbed the bug initially, because it looked
like
a loose end and a simple thing to address by fulfilling the original
request to propagate the visibility up to the Perf.java code.
Let's see if other reviewers have similar views - that the UsePerfData
flag can be removed. This bug was originally filed against 8u60 and is
currently targetted for 12. I only explored a potential fix at this time
in case we need to get a deprecation process started at this time.
...
