JDK-8069149: jdk.internal.pref.Perf is unaware of the VM option UsePerfData
David Holmes
david.holmes at oracle.com
Sun Jun 3 22:50:03 UTC 2018
Hi Gary,
Thanks for the further explanations, I have a much better view of things
now. To summarise:
- The VM initializes the perfMemory subsystem during early VM initialization
- If !UsePerfdata then perfMemory is not initialized.
- The initialization state of the perfMemory system is exposed to the
Java Perf class via a local initialization flag set at class init time
- The Perf class API checks for initialization (either at the Java
level, or in native code) and throws IOException if not initialized.
- The users of the Perf class can check for IOExceptions.
There was a question as to whether the Perf API should deal with the
initialization issue and throw exceptions, or whether the obtaining of
the Perf instance should throw the exception. I think the API level is
fine as the time related parts of the API still function even if the
rest doesn't.
Looking at the changes again ...
I see four clients of the Perf instance:
1. ./java.base/share/classes/jdk/internal/perf/PerfCounter.java
This is the primary API used by the rest of the JDK libraries to
interact with the "perf" systems. The counters must not throw any
exceptions if the Perf system is not available due to !UsePerfData
(which they don't). It's unclear how users of the counters may be
impacted if the "counts" always return zero ??
Suggestion: it might be useful if PerfCounter.toString showed if the
Perf system was disabled e.g.
return name + (perf.isInitialized() ? "" : "[Disabled]" + " = " + get();
2. ./java.management/share/classes/sun/management/VMManagementImpl.java
This code is not modified by your webrev but seems to have its own
expectations about the impact of !UsePerfdata:
} catch (IllegalArgumentException e) {
// If the shared memory doesn't exist e.g. if -XX:-UsePerfData
// was set
noPerfData = true;
} catch (IOException e) {
throw new AssertionError(e);
}
I can't see where/why this code thinks that -UsePerfData leads to
IllegalArgumentException, but your throwing of IOException is going to
break it badly! This needs further investigation.
3.
./jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/local/PerfDataBuffer.java
This class throws exceptions on any attach failure so in that sense the
IOException doesn't change that, however it does appear that this class
is intended to throw MonitorException if things go wrong. More generally
we should look at how the jvmstat tool(s?) are used and whether they are
reporting the lack of UsePerfData in a user-friendly way.
4.
./jdk.management.agent/share/classes/jdk/internal/agent/ConnectorAddressLink.java
I'm not at all clear on the use of this class so it is hard to assess
the change. The throwing of IOException from the "import" function is
fine, but the silent inaction of the "export" functions is less clear.
---
On the issue of IOException and message ... I think the native code that
throws IOException when not-initialized should use the same message as
the Java code. That's the only consistent thing to do IMHO. It would
also be helpful to the user if the exception message could tie back to
the -UsePerfData setting. I don't normally suggest things like this but
these failures (as we found out in SE Embedded days when we did turn
this off!) can be somewhat perplexing to the end user. To that end perhaps:
throw new IOException("Perf memory is not initialized - check
UsePerfData setting");
---
On the meta questions ... it seems the ability to disable UsePerfData
can have a significant affect on startup so may be relied upon by some
users. So any change to this would need to go through a rigorous process
to determine impact. So I'd say that's future work beyond the scope of
the current RFE/bug.
Thanks,
David
On 1/06/2018 11:06 PM, Gary Adams wrote:
> 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.
>
> ...
More information about the serviceability-dev
mailing list