JDK-8069149: jdk.internal.pref.Perf is unaware of the VM option UsePerfData
Gary Adams
gary.adams at oracle.com
Fri Jun 1 13:06:13 UTC 2018
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.
...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180601/0a7ce335/attachment.html>
More information about the serviceability-dev
mailing list