RFR (M): 8207266: ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
Hohensee, Paul
hohensee at amazon.com
Wed Sep 18 23:47:02 UTC 2019
I'll take a look.
On 9/18/19, 4:40 PM, "David Holmes" <david.holmes at oracle.com> wrote:
Paul,
Unfortunately this patch has broken the vmTestbase/nsk/monitoring tests:
[2019-09-18T22:59:32,349Z]
/scratch/mesos/jib-master/install/jdk-14+15-615/src.full/open/test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/server/ServerThreadMXBeanNew.java:32:
error: ServerThreadMXBeanNew is not abstract and does not override
abstract method getCurrentThreadAllocatedBytes() in ThreadMXBean
and possibly other issues as we are seeing hundreds of failures.
David
On 18/09/2019 8:50 am, David Holmes wrote:
> On 18/09/2019 12:10 am, Hohensee, Paul wrote:
>> Thanks, Serguei. :)
>>
>> David, are you ok with the patch?
>
> Yep, nothing further from me.
>
> David
>
>> Paul
>>
>> *From: *"serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> *Date: *Tuesday, September 17, 2019 at 2:26 AM
>> *To: *"Hohensee, Paul" <hohensee at amazon.com>, David Holmes
>> <david.holmes at oracle.com>, Mandy Chung <mandy.chung at oracle.com>
>> *Cc: *OpenJDK Serviceability <serviceability-dev at openjdk.java.net>,
>> "hotspot-gc-dev at openjdk.java.net" <hotspot-gc-dev at openjdk.java.net>
>> *Subject: *Re: RFR (M): 8207266:
>> ThreadMXBean::getThreadAllocatedBytes() can be quicker for self thread
>>
>> Hi Paul,
>>
>> Thank you for refactoring and fixing the test.
>> It looks great now!
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/15/19 02:52, Hohensee, Paul wrote:
>>
>> Hi, Serguei, thanks for the review. New webrev at
>>
>> http://cr.openjdk.java.net/~phh/8207266/webrev.09/
>>
>> I refactored the test’s main() method, and you’re correct,
>> getThreadAllocatedBytes should be getCurrentThreadAllocatedBytes in
>> that context: fixed.
>>
>> Paul
>>
>> *From: *"serguei.spitsyn at oracle.com"
>> <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com>
>> <mailto:serguei.spitsyn at oracle.com>
>> *Organization: *Oracle Corporation
>> *Date: *Friday, September 13, 2019 at 5:50 PM
>> *To: *"Hohensee, Paul" <hohensee at amazon.com>
>> <mailto:hohensee at amazon.com>, David Holmes <david.holmes at oracle.com>
>> <mailto:david.holmes at oracle.com>, Mandy Chung
>> <mandy.chung at oracle.com> <mailto:mandy.chung at oracle.com>
>> *Cc: *OpenJDK Serviceability <serviceability-dev at openjdk.java.net>
>> <mailto:serviceability-dev at openjdk.java.net>,
>> "hotspot-gc-dev at openjdk.java.net"
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>> <hotspot-gc-dev at openjdk.java.net>
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>> *Subject: *Re: RFR (M): 8207266:
>> ThreadMXBean::getThreadAllocatedBytes() can be quicker for self
>> thread
>>
>> Hi Paul,
>>
>> It looks pretty good in general.
>>
>>
>> http://cr.openjdk.java.net/~phh/8207266/webrev.08/test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java.frames.html
>>
>>
>> It would be nice to refactor the java main() method as it becomes
>> too big.
>> Two ways ofgetCurrentThreadAllocatedBytes() testing are good
>> candidates
>> to become separate methods.
>>
>> 98 long size1 = mbean.getThreadAllocatedBytes(id);
>>
>> Just wanted to double check if you wanted to invoke
>> the getCurrentThreadAllocatedBytes() instead as it is
>> a part of:
>>
>> 85 // First way, getCurrentThreadAllocatedBytes
>>
>>
>> Thanks,
>> Serguei
>>
>> On 9/13/19 12:11 PM, Hohensee, Paul wrote:
>>
>> Hi David, thanks for your comments. New webrev in
>>
>>
>> http://cr.openjdk.java.net/~phh/8207266/webrev.08/
>>
>>
>> Both the old and new versions of the code check that thread
>> allocated memory is both supported and enabled. The existing version
>> of getThreadAllocatedBytes(long []) calls
>> verifyThreadAllocatedMemory(long []), which checks inline to make sure
>> thread allocated memory is supported, then calls
>> isThreadAllocatedMemoryEnabled() to verify that it's enabled.
>> isThreadAllocatedMemoryEnabled() duplicates (!) the support check and
>> returns the enabled flag. I removed the redundant check in the new
>> version.
>>
>>
>> You're of course correct about the back-to-back check.
>> Application code can't know when the runtime will hijack a thread for
>> its own purposes. I've removed the check.
>>
>>
>> Paul
>>
>>
>> On 9/13/19, 12:50 AM, "David Holmes"<david.holmes at oracle.com>
>> <mailto:david.holmes at oracle.com> wrote:
>>
>>
>> Hi Paul,
>>
>>
>> On 13/09/2019 10:29 am, Hohensee, Paul wrote:
>>
>> > Thanks for clarifying the review rules. Would someone
>> from the
>>
>> > serviceability team please review? New webrev at
>>
>> >
>>
>> >http://cr.openjdk.java.net/~phh/8207266/webrev.07/
>>
>>
>> One aspect of the functional change needs clarification
>> for me - and
>>
>> apologies if this has been covered in the past. It seems
>> to me that
>>
>> currently we only check isThreadAllocatedMemorySupported
>> for these
>>
>> operations, but if I read things correctly the updated
>> code additionally
>>
>> checks isThreadAllocatedMemoryEnabled, which is a
>> behaviour change not
>>
>> mentioned in the CSR.
>>
>>
>> > I didn’t disturb the existing checks in the test, just
>> added code to
>>
>> > check the result of getThreadAllocatedBytes(long) on a
>> non-current
>>
>> > thread, plus the back-to-back no-allocation checks. The
>> former wasn’t
>>
>> > needed before because getThreadAllocatedBytes(long) was
>> just a wrapper
>>
>> > around getThreadAllocatedBytes(long []). This patch
>> changes that, so I
>>
>> > added a separate test. The latter is supposed to fail
>> if there’s object
>>
>> > allocation on calls to getCurrentThreadAllocatedBytes and
>>
>> > getThreadAllocatedBytes(long). I.e., a feature, not a
>> bug, because
>>
>> > accumulation of transient small objects can be a
>> performance problem.
>>
>> > Thanks to your review, I noticed that the back-to-back
>> check on the
>>
>> > current thread was using getThreadAllocatedBytes(long)
>> instead of
>>
>> > getCurrentThreadAllocatedBytes and fixed it. I also
>> removed all
>>
>> > instances of “TEST FAILED: “.
>>
>>
>> The back-to-back check is not valid in general. You don't
>> know if the
>>
>> first check might trigger some class loading on the
>> return path after it
>>
>> has obtained the first memory value. The check might also
>> fail if using
>>
>> JVMCI and some compilation related activity occurs in the
>> current thread
>>
>> on the second call. Also with the introduction of
>> handshakes its
>>
>> possible the current thread might hit a safepoint checks
>> that results in
>>
>> it executing a handshake operation that performs
>> allocation. Potentially
>>
>> there could be numerous non-deterministic actions that
>> might occur
>>
>> leading to unanticipated allocation.
>>
>>
>> I understand what you want to test here, I just don't
>> think it is
>>
>> reliably doable.
>>
>>
>> Thanks,
>>
>> David
>>
>> -----
>>
>>
>> >
>>
>> > Paul
>>
>> >
>>
>> > *From: *Mandy Chung<mandy.chung at oracle.com>
>> <mailto:mandy.chung at oracle.com>
>>
>> > *Date: *Thursday, September 12, 2019 at 10:09 AM
>>
>> > *To: *"Hohensee, Paul"<hohensee at amazon.com>
>> <mailto:hohensee at amazon.com>
>>
>> > *Cc: *OpenJDK
>> Serviceability<serviceability-dev at openjdk.java.net>
>> <mailto:serviceability-dev at openjdk.java.net>,
>>
>> >"hotspot-gc-dev at openjdk.java.net"
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>> <hotspot-gc-dev at openjdk.java.net>
>> <mailto:hotspot-gc-dev at openjdk.java.net>
>>
>> > *Subject: *Re: RFR (M): 8207266:
>> ThreadMXBean::getThreadAllocatedBytes()
>>
>> > can be quicker for self thread
>>
>> >
>>
>> > On 9/3/19 12:38 PM, Hohensee, Paul wrote:
>>
>> >
>>
>> > Minor update in new
>> webrevhttp://cr.openjdk.java.net/~phh/8207266/webrev.05/.
>>
>> >
>>
>> >
>>
>> > I only reviewed the library side implementation that
>> looks good. I
>>
>> > expect the serviceability team to review the test and
>> hotspot change.
>>
>> >
>>
>> >
>>
>> > Need a confirmatory review to push this. If I
>> understand the rules correctly, it doesn't need a Reviewer review
>> since Mandy's already reviewed it, it just needs a Committer review.
>>
>> >
>>
>> >
>>
>> > You need another reviewer to advice the following
>> because I was not
>>
>> > close to the ThreadsList work.
>>
>> >
>>
>> > 2087 ThreadsListHandle tlh;
>>
>> >
>>
>> > 2088 JavaThread* java_thread =
>> tlh.list()->find_JavaThread_from_java_tid(thread_id);
>>
>> >
>>
>> > 2089
>>
>> >
>>
>> > 2090 if (java_thread != NULL) {
>>
>> >
>>
>> > 2091 return java_thread->cooked_allocated_bytes();
>>
>> >
>>
>> > 2092 }
>>
>> >
>>
>> > This looks right to me.
>>
>> >
>>
>> >
>> test/jdk/com/sun/management/ThreadMXBean/ThreadAllocatedMemory.java
>>
>> >
>>
>> > - "ThreadAllocatedMemory is expected to
>> be disabled");
>>
>> >
>>
>> > + "TEST FAILED: ThreadAllocatedMemory is
>> expected to be
>>
>> > disabled");
>>
>> >
>>
>> > Prepending "TEST FAILED" in exception message (in
>> several places)
>>
>> >
>>
>> > seems redundant since such RuntimeException is thrown
>> and expected
>>
>> >
>>
>> > a test failure.
>>
>> >
>>
>> > + // back-to-back calls shouldn't allocate any
>> memory
>>
>> >
>>
>> > + size = mbean.getThreadAllocatedBytes(id);
>>
>> >
>>
>> > + size1 = mbean.getThreadAllocatedBytes(id);
>>
>> >
>>
>> > + if (size1 != size) {
>>
>> >
>>
>> > Is there anything in the test can do to help guarantee
>> this? I didn't
>>
>> >
>>
>> > closely review this test. The main thing I advice is
>> to improve
>>
>> >
>>
>> > the reliability of this test. Put it in another way,
>> we want to
>>
>> >
>>
>> > ensure that this test change will pass all the time in
>> various
>>
>> >
>>
>> > test configuration.
>>
>> >
>>
>> > Mandy
>>
>> >
>>
>>
>>
>>
>>
More information about the serviceability-dev
mailing list