Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v7]

[Stefan Karlsson]

On Tue, 26 Apr 2022 17:27:35 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 8d8f0a2fd646e57fe6b4e8ab669f836dc46dda69

Similar comments to @fisk.

This is what I've been focusing on:
* In-depth:  gc/, oops/, memory/, utilities/
* Partially: runtime/
* Cleanups: hotspot/

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8285364: Remove REF_ enum for java.lang.ref.Reference [v3]

[Stefan Karlsson]

On Mon, 25 Apr 2022 15:51:35 GMT, Coleen Phillimore  wrote:

>> Albert Mingkun Yang has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove REF_ enum for java.lang.ref.Reference
>>   
>>   Signed-off-by: Albert Yang 
>
> src/hotspot/share/oops/instanceKlass.cpp line 441:
> 
>> 439: 
>> 440:   // Allocation
>> 441:   if (parser.is_java_lang_ref_Reference_subclass()) {
> 
> I'm having a really hard time understanding this.  java.lang.Reference now 
> doesn't have any REF_OTHER type set.  I didn't realize that a 
> java.lang.Reference instance is a plain InstanceKlass, and not an 
> InstanceRefKlass.  Is this right?

Yes, I was also surprised by this. The java.lang.ref.Reference Klass is only an 
InstanceKlass, not an InstanceRefKlass.

-

PR: https://git.openjdk.java.net/jdk/pull/8332

Re: RFR: 8285364: Remove REF_ enum for java.lang.ref.Reference [v2]

[Stefan Karlsson]

On Fri, 22 Apr 2022 09:04:37 GMT, Kim Barrett  wrote:

> > > Using REF_SOFT seems too hacky.
> > 
> > 
> > Just to put all alternatives on the table. The use of `REF_SOFT` is 
> > ephemeral.
> > [...]
> > I have no particular preference. What does everyone think?
> 
> I also think using REF_SOFT like that is overly hacky.
> 
> I think there is still a bootstrapping weirdness, whether using REF_REFERENCE 
> or using Stefan's approach. I think Soft/Weak/Final/PhantomReference_klass 
> get constructed with the reftype of Reference_klass (whatever that is), and 
> then have it later updated to the correct value. In that respect REF_OTHER 
> seems better than REF_REFERENCE. So I no longer like the renaming. Getting 
> rid of it entirely with Stefan's approach, they get REF_NONE initially and 
> then set to the proper value; that's far from the worst bootstrapping kludge 
> I've ever seen.

I think we can fix the bootstrapping kludge with something like this:
https://github.com/stefank/jdk/commit/e62ddb780730dbb5d6766f882346bf0fcc6cdb59

With this patch, we'll always have the correct _reference_type after the 
InstanceRefKlass constructor has run.

(Patch has not gone through full testing yet)

-

PR: https://git.openjdk.java.net/jdk/pull/8332

Re: RFR: 8285364: Use more precise name for ReferenceType::REF_OTHER [v2]

[Stefan Karlsson]

On Thu, 21 Apr 2022 11:30:20 GMT, Albert Mingkun Yang  wrote:

>> Simple rename and some comments update.
>> 
>> Test: build
>
> Albert Mingkun Yang has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review

Using REF_SOFT seems too hacky. If we rewrite the initialization a bit we can 
make this work. See suggested change in:
https://github.com/stefank/jdk/tree/ref-rename
https://github.com/stefank/jdk/commit/68161ce45de7d35b81f9cd6414e1e4cdf0c07665

-

PR: https://git.openjdk.java.net/jdk/pull/8332

Re: RFR: 8285364: Use more precise name for ReferenceType::REF_OTHER [v2]

[Stefan Karlsson]

On Thu, 21 Apr 2022 11:30:20 GMT, Albert Mingkun Yang  wrote:

>> Simple rename and some comments update.
>> 
>> Test: build
>
> Albert Mingkun Yang has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review

That's unfortunate. The allocate_instance_klass function can easily be changed 
to also check if class_name == vmSymbols::java_lang_ref_Reference(). But even 
with that, make images fails.

I wasn't a fan of the REF_REFERENCE name, since it isn't really that 
descriptive to me, and had hoped to fine a way to just get rid of it instead of 
finding a better name.

-

PR: https://git.openjdk.java.net/jdk/pull/8332

Re: RFR: 8285364: Use more precise name for ReferenceType::REF_OTHER [v2]

[Stefan Karlsson]

On Thu, 21 Apr 2022 11:30:20 GMT, Albert Mingkun Yang  wrote:

>> Simple rename and some comments update.
>> 
>> Test: build
>
> Albert Mingkun Yang has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review

Do we even need REF_OTHER / REF_REFERENCE? I see a bunch of ShouldNotReachHeres 
and then set_reference_type(REF_OTHER). Maybe the latter could be changed to 
REF_NONE?

-

PR: https://git.openjdk.java.net/jdk/pull/8332

Re: RFR: 8284903: Fix typos in hotspot

[Stefan Karlsson]

On Fri, 15 Apr 2022 07:40:04 GMT, Magnus Ihse Bursie  wrote:

> I ran `codespell` on hotspot, and accepted those changes where it indeed 
> discovered real typos. 
> 
> You'd be surprised over the many implementions of instrinsics and other 
> intructions accross all archtectures I've encounted, so for the preceding 
> reason it's neccesery to sucessfully seach for exisiting typos...

Reviewed:
- gc/
- oops/
- utilities/

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8260

Re: RFR: 8276696: ParallelObjectIterator freed at the wrong time in VM_HeapDumper

[Stefan Karlsson]

On Mon, 22 Nov 2021 13:49:02 GMT, Erik Österlund  wrote:

> The VM_HeapDumper code uses a C heap allocated ParallelObjectIterator. It is 
> constructed right before running a parallel operation with a work gang, but 
> freed in the destructor of the VM_HeapDumper. This means it is created on one 
> thread and deleted on another thread. This becomes a bit problematic when a 
> parallel object iterator implementation uses a ThreadsListHandle (which is 
> indeed the case for ZGC). This patch changes ParallelObjectIterator to be a 
> StackObj, carrying a ParallelObjectIteratorImpl object, which is never 
> exposed publicly. This ensures that construction and destruction of the 
> internal object iterator is scoped like RAII objects, hence complying with 
> how ThreadsListHandle is supposed to be used.

Marked as reviewed by stefank (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6501

Re: RFR: 8275035: Clean up worker thread infrastructure

[Stefan Karlsson]

On Mon, 11 Oct 2021 09:21:21 GMT, Per Liden  wrote:

> I propose that we clean up our GangWorker/WorkGang and related classes, to 
> remove abstractions we no longer need (after CMS was removed, MutexDispatcher 
> was removed, Parallel is now using WorkGang, etc) and adjusting names as 
> follows:
> 
> * Rename AbstractGangTask to WorkerTask
> * Rename WorkGang to WorkerThreads
> * Fold GangWorker into WorkerThread
> * Fold WorkManager into WorkerThreads
> * Move SubTaskDone and friends to a new workerUtils.hpp/cpp
> 
> I've split things up into several commits to make it easier to review.
> 
> Testing: Passes Tier 1-3 on all Oracle platforms.

Looks good

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5886

Re: libjdwp crash while debugging application ran on OpenJDK 16

[Stefan Karlsson]

Hi Simeon,

On 2021-06-23 12:39, S A wrote:

Hi all,

my colleagues recently ran into a crash in libjdwp, when measuring
performance with OpenJDK 17 (early access build). The same crash was
observed with OpenJDK 16.0.1, but not with OpenJDK 15.

We are hoping to get a fix for the crash, before the official OpenJDK 17
release. We've opened a RHEL bugzilla ticket for this (
https://bugzilla.redhat.com/show_bug.cgi?id=1972529), but we expect this
won't be enough to resolve the crash before the release.

What would the appropriate medium be to report the problem? I assume this
mailing list is not the right place, but I don't know how to otherwise
reach out to OpenJDK developers and maintainers (other than via RHEL
support/bugzilla, which is often a slow process).


The hs_err crash files usually states where to report the bugs. 
Oracle-built binaries say:

# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp

I think serviceability-dev would be the be the correct mailing list. 
I'll bcc away jdk-dev and will move this to serviceablity-dev.


I wonder if this could be caused by calling commonRef_unpin on a ref 
that is not "pinned"? Specifically, look at the updated weakenNode:


static jweak
weakenNode(JNIEnv *env, RefNode *node)
{
    if (node->strongCount == 1) {
...
    return weakRef;
    } else {
    node->strongCount--;
    return node->ref;
    }
}

if strongCount is 0, this will underflow and then delete node will take 
the wrong path:


if (node->strongCount != 0) {
  JNI_FUNC_PTR(env,DeleteGlobalRef)(env, node->ref);
} else {
  JNI_FUNC_PTR(env,DeleteWeakGlobalRef)(env, node->ref);
}

The previous version of weakenNode looked like this:

static jweak
weakenNode(JNIEnv *env, RefNode *node)
{
    if (node->isStrong) {
...
    return weakRef;
    } else {
    return node->ref;
    }
}

so a unbalanced unpin call would previously not fail in the same way.

StefanK



Sorry for the noise on the mailing list.

Best regards,
Simeon

*Extra infos and background:*

We would like to move to OpenJDK 17, in order to use Shenandoah GC.
Shenandoah GC shows promising results for our product, though only with the
fix for https://bugs.openjdk.java.net/browse/JDK-8254315. We would not be
able to switch (or even evaluate the switch properly) until the crash is
resolved.

Unfortunately we have no reproducer outside of our product, and our
reproducer is quite complex. Trivial use cases of debugging OpenJDK 16 with
Eclipse (we base our product on Eclipse) do not run into a crash.

A fast debug build for OpenJDK 16 reports an assertion fail as follows:

#  Internal Error
(/tmp/jep2604_fastdebug_jdk16/jdk16/src/hotspot/share/runtime/jniHandles.cpp:148),
pid=81289, tid=56880
#  assert(!is_jweak(handle)) failed: wrong method for detroying jweak
#
# JRE version: OpenJDK Runtime Environment (16.0) (fastdebug build
16-internal+0-adhoc.sandreev.jdk16)
# Java VM: OpenJDK 64-Bit Server VM (fastdebug
16-internal+0-adhoc.sandreev.jdk16, mixed mode, tiered, compressed oops, g1
gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x1043cb4]  JNIHandles::destroy_global(_jobject*)+0x224
...
Stack: [0x7ffe3c4f5000,0x7ffe3c5f6000],  sp=0x7ffe3c5f4be0,
  free space=1022k
Native frames: (J=compiled Java code, A=aot compiled Java code,
j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x1043cb4]  JNIHandles::destroy_global(_jobject*)+0x224
V  [libjvm.so+0xfafd9f]  jni_DeleteGlobalRef+0x10f
C  [libjdwp.so+0xf11a]  deleteNode+0x7a
C  [libjdwp.so+0xf71f]  commonRef_reset+0x6f
C  [libjdwp.so+0x10dec]  debugInit_reset+0x6c
C  [libjdwp.so+0x270a2]  acceptThread+0xa2
V  [libjvm.so+0x127c054]  JvmtiAgentThread::call_start_function()+0x1d4
V  [libjvm.so+0x1a51e9a]  JavaThread::thread_main_inner()+0x5ba
V  [libjvm.so+0x1a589e0]  Thread::call_run()+0x100
V  [libjvm.so+0x15fbd26]  thread_native_entry(Thread*)+0x116

Going through the stack trace, looking for related changes, I've not been
able to reproduce the crash with this commit reverted:
https://github.com/openjdk/jdk/commit/79f1dfb8d3941377da77e73f7bbab93baef29b8e

Re: RFR: 8268368: Adopt cast notation for JavaThread conversions [v2]

[Stefan Karlsson]

On Tue, 22 Jun 2021 07:17:00 GMT, Guoxiong Li  wrote:

>> Hi all,
>> 
>> Considering the consistency of `JavaThread` and other threads, such as 
>> WorkerThread and CompilerThread, `JavaThread` could use a method named 
>> `cast` to replace the method `Thread::as_Java_thread()`. It can reduce the 
>> Thread's knowledge about the subtypes.
>> 
>> This patch removes two methods, `JavaThread* Thread::as_Java_thread()` and 
>> `const JavaThread* Thread::as_Java_thread() const`, of the class `Thread` 
>> and adds two static methods, `JavaThread* cast(Thread* t)` and `const 
>> JavaThread* cast(const Thread* t)`, to the class `JavaThread`. 
>> Correspondingly, the code of the method `JavaThread::current()` need to be 
>> adjusted and many places where the method `Thread::as_Java_thread()` is used 
>> need to use `JavaThread::cast` instead.
>> 
>> Test:
>> tier1 passed locally.
>> 
>> Thanks for taking the time to review.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix incorrect use of the method cast

Marked as reviewed by stefank (Reviewer).

src/hotspot/share/runtime/thread.hpp line 1432:

> 1430: assert(t->is_Java_thread(), "incorrect cast to const JavaThread");
> 1431: return static_cast(t);
> 1432:   }

Now that you've written the code in-place, you could  remove the `inline` 
specifier.

-

PR: https://git.openjdk.java.net/jdk/pull/4546

Integrated: 8267464: Circular-dependency resilient inline headers

[Stefan Karlsson]

On Thu, 20 May 2021 11:55:05 GMT, Stefan Karlsson  wrote:

> I'd like to propose a small adjustment to how we write .inline.hpp files, in 
> the hope to reduce include problems because of circular dependencies between 
> inline headers.
> 
> This is a small, contrived example to show the problem:
> 
> // a.hpp
> #pragma once
> 
> void a1();
> void a2();
> 
> 
> // a.inline.hpp
> #pragma once
> 
> #include "a.hpp"
> #include "b.inline.hpp"
> 
> inline void a1() {
>   b1();
> }
> 
> inline void a2() {
> }
> 
> 
> // b.hpp
> #pragma once
> 
> void b1();
> void b2();
> 
> 
> // b.inline.hpp
> #pragma once
> 
> #include "a.inline.hpp"
> #include "b.hpp"
> 
> inline void b1() {
> }
> 
> inline void b2() {
>   a1();
> }
> 
> The following code compiles fine:
> 
> // a.cpp
> #include "a.inline.hpp"
> 
> int main() {
>   a1();
> 
>   return 0;
> }
> 
> But the following:
> 
> // b.cpp
> #include "b.inline.hpp"
> 
> int main() {
>   b1();
> 
>   return 0;
> }
> 
> 
> fails with the this error message:
> 
> In file included from b.inline.hpp:3,
>  from b.cpp:1:
> a.inline.hpp: In function ‘void a1()’:
> a.inline.hpp:8:3: error: ‘b1’ was not declared in this scope; did you mean 
> ‘a1’?
> 
> We can see the problem with g++ -E:
> 
> # 1 "a.inline.hpp" 1
> # 1 "a.hpp" 1
> 
> void a1();
> void a2();
> 
> # 4 "a.inline.hpp" 2
> 
> inline void a1() {
>   b1();
> }
> 
> inline void a2() {
> }
> 
> # 4 "b.inline.hpp" 2
> # 1 "b.hpp" 1
> 
> void b1();
> void b2();
> 
> # 5 "b.inline.hpp" 2
> 
> inline void b1() {
> }
> 
> inline void b2() {
>   a1();
> }
> 
> # 2 "b.cpp" 2
> 
> int main() {
>   b1();
> 
>   return 0;
> }
> 
> 
> b1() is called before it has been declared. b.inline.hpp inlined 
> a.inline.hpp, which uses functions declared in b.hpp. However, at that point 
> we've not included enough of b.inline.hpp to have declared b1().
> 
> This is a small and easy example. In the JVM the circular dependencies 
> usually involves more than two files, and often from different sub-systems of 
> the JVM. We have so-far solved this by restructuring the code, making 
> functions out-of-line, creating proxy objects, etc. However, the more we use 
> templates, the more this is going to become a reoccurring nuisance. And in 
> experiments with lambdas, which requires templates, this very quickly showed 
> up as a problem.
> 
> I propose that we solve most (all?) of these issues by adding a rule that 
> states that .inline.hpp files should start by including the corresponding 
> .hpp, and that the .hpp should contain the declarations of all externally 
> used parts of the .inline.hpp file. This should guarantee that the 
> declarations are always present before any subsequent include can create a 
> circular include dependency back to the original file.
> 
> In the example above, b.inline.hpp would become:
> 
> // b.inline.hpp
> #pragma once
> 
> #include "b.hpp"
> #include "a.inline.hpp"
> 
> inline void b1() {
> }
> 
> inline void b2() {
>   a1();
> }
> 
> and now both a.cpp and b.cpp compiles. The generated output now looks like 
> this:
> 
> # 1 "b.inline.hpp" 1
> # 1 "b.hpp" 1
> 
> void b1();
> void b2();
> 
> # 4 "b.inline.hpp" 2
> # 1 "a.inline.hpp" 1
> 
> # 1 "a.hpp" 1
> 
> void a1();
> void a2();
> 
> # 4 "a.inline.hpp" 2
> 
> inline void a1() {
>   b1();
> }
> 
> inline void a2() {
> }
> 
> # 5 "b.inline.hpp" 2
> 
> inline void b1() {
> }
> 
> inline void b2() {
>   a1();
> }
> 
> # 2 "b.cpp" 2
> 
> int main() {
>   b1();
> 
>   return 0;
> }
> 
> The declarations come first, and the compiler is happy. 
> 
> An alternative to this, that has been proposed by other HotSpot GC devs have 
> been to always include all .hpp files first, and then have a section of 
> .inline.hpp includes. I've experimented with that as well, but I think it 
> requires more maintenance and vigilance of  *users* .inline.hpp files (add 
> .hpp include to the first section, add .inline.hpp section to second). The 
> proposed structures only requires extra care from *writers* of .inline.hpp 
> files. All others can include .hpp and .inline.hpp as we've been doing for a 
> long time now.
> 
> Some 

Re: RFR: 8267464: Circular-dependency resilient inline headers [v8]

[Stefan Karlsson]

On Mon, 31 May 2021 06:44:45 GMT, Stefan Karlsson  wrote:

>> I'd like to propose a small adjustment to how we write .inline.hpp files, in 
>> the hope to reduce include problems because of circular dependencies between 
>> inline headers.
>> 
>> This is a small, contrived example to show the problem:
>> 
>> // a.hpp
>> #pragma once
>> 
>> void a1();
>> void a2();
>> 
>> 
>> // a.inline.hpp
>> #pragma once
>> 
>> #include "a.hpp"
>> #include "b.inline.hpp"
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> 
>> // b.hpp
>> #pragma once
>> 
>> void b1();
>> void b2();
>> 
>> 
>> // b.inline.hpp
>> #pragma once
>> 
>> #include "a.inline.hpp"
>> #include "b.hpp"
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> The following code compiles fine:
>> 
>> // a.cpp
>> #include "a.inline.hpp"
>> 
>> int main() {
>>   a1();
>> 
>>   return 0;
>> }
>> 
>> But the following:
>> 
>> // b.cpp
>> #include "b.inline.hpp"
>> 
>> int main() {
>>   b1();
>> 
>>   return 0;
>> }
>> 
>> 
>> fails with the this error message:
>> 
>> In file included from b.inline.hpp:3,
>>  from b.cpp:1:
>> a.inline.hpp: In function ‘void a1()’:
>> a.inline.hpp:8:3: error: ‘b1’ was not declared in this scope; did you mean 
>> ‘a1’?
>> 
>> We can see the problem with g++ -E:
>> 
>> # 1 "a.inline.hpp" 1
>> # 1 "a.hpp" 1
>> 
>> void a1();
>> void a2();
>> 
>> # 4 "a.inline.hpp" 2
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> # 4 "b.inline.hpp" 2
>> # 1 "b.hpp" 1
>> 
>> void b1();
>> void b2();
>> 
>> # 5 "b.inline.hpp" 2
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> # 2 "b.cpp" 2
>> 
>> int main() {
>>   b1();
>> 
>>   return 0;
>> }
>> 
>> 
>> b1() is called before it has been declared. b.inline.hpp inlined 
>> a.inline.hpp, which uses functions declared in b.hpp. However, at that point 
>> we've not included enough of b.inline.hpp to have declared b1().
>> 
>> This is a small and easy example. In the JVM the circular dependencies 
>> usually involves more than two files, and often from different sub-systems 
>> of the JVM. We have so-far solved this by restructuring the code, making 
>> functions out-of-line, creating proxy objects, etc. However, the more we use 
>> templates, the more this is going to become a reoccurring nuisance. And in 
>> experiments with lambdas, which requires templates, this very quickly showed 
>> up as a problem.
>> 
>> I propose that we solve most (all?) of these issues by adding a rule that 
>> states that .inline.hpp files should start by including the corresponding 
>> .hpp, and that the .hpp should contain the declarations of all externally 
>> used parts of the .inline.hpp file. This should guarantee that the 
>> declarations are always present before any subsequent include can create a 
>> circular include dependency back to the original file.
>> 
>> In the example above, b.inline.hpp would become:
>> 
>> // b.inline.hpp
>> #pragma once
>> 
>> #include "b.hpp"
>> #include "a.inline.hpp"
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> and now both a.cpp and b.cpp compiles. The generated output now looks like 
>> this:
>> 
>> # 1 "b.inline.hpp" 1
>> # 1 "b.hpp" 1
>> 
>> void b1();
>> void b2();
>> 
>> # 4 "b.inline.hpp" 2
>> # 1 "a.inline.hpp" 1
>> 
>> # 1 "a.hpp" 1
>> 
>> void a1();
>> void a2();
>> 
>> # 4 "a.inline.hpp" 2
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> # 5 "b.inline.hpp" 2
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> # 2 "

Re: RFR: 8267464: Circular-dependency resilient inline headers [v8]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains eight commits:

 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Review kbarrett
 - Review dholmes
 - Update HotSpot style guide documents
 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Clean up assembler_.inline.hpp
 - 8267464: Circular-dependency resiliant inline headers

-

Changes: https://git.openjdk.java.net/jdk/pull/4127/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4127=07
  Stats: 490 lines in 242 files changed: 349 ins; 118 del; 23 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267842: SIGSEGV in get_current_contended_monitor [v6]

[Stefan Karlsson]

On Fri, 28 May 2021 13:42:30 GMT, Martin Doerr  wrote:

>> We need a fix for crashes in get_current_contended_monitor due to concurrent 
>> modification of memory locations which are not declared volatile. See bug 
>> for details.
>
> Martin Doerr has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Improve comments.

Looks good. Thanks!

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4224

Re: RFR: 8267464: Circular-dependency resilient inline headers [v7]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains seven commits:

 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Review kbarrett
 - Review dholmes
 - Update HotSpot style guide documents
 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Clean up assembler_.inline.hpp
 - 8267464: Circular-dependency resiliant inline headers

-

Changes: https://git.openjdk.java.net/jdk/pull/4127/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4127=06
  Stats: 490 lines in 242 files changed: 349 ins; 118 del; 23 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8265148: StackWatermarkSet being updated during AsyncGetCallTrace

[Stefan Karlsson]

On Thu, 27 May 2021 04:01:17 GMT, Leonid Mesnik  wrote:

> 8265148: StackWatermarkSet being updated during AsyncGetCallTrace

Looks good. Could you verify the fix with ZGC? G1 doesn't use it, so testing 
with G1 will only show that we don't hit the failing assert anymore.

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4217

Re: RFR: 8267464: Circular-dependency resilient inline headers [v6]

[Stefan Karlsson]

On Wed, 26 May 2021 18:00:54 GMT, Stefan Karlsson  wrote:

>> I'd like to propose a small adjustment to how we write .inline.hpp files, in 
>> the hope to reduce include problems because of circular dependencies between 
>> inline headers.
>> 
>> This is a small, contrived example to show the problem:
>> 
>> // a.hpp
>> #pragma once
>> 
>> void a1();
>> void a2();
>> 
>> 
>> // a.inline.hpp
>> #pragma once
>> 
>> #include "a.hpp"
>> #include "b.inline.hpp"
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> 
>> // b.hpp
>> #pragma once
>> 
>> void b1();
>> void b2();
>> 
>> 
>> // b.inline.hpp
>> #pragma once
>> 
>> #include "a.inline.hpp"
>> #include "b.hpp"
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> The following code compiles fine:
>> 
>> // a.cpp
>> #include "a.inline.hpp"
>> 
>> int main() {
>>   a1();
>> 
>>   return 0;
>> }
>> 
>> But the following:
>> 
>> // b.cpp
>> #include "b.inline.hpp"
>> 
>> int main() {
>>   b1();
>> 
>>   return 0;
>> }
>> 
>> 
>> fails with the this error message:
>> 
>> In file included from b.inline.hpp:3,
>>  from b.cpp:1:
>> a.inline.hpp: In function ‘void a1()’:
>> a.inline.hpp:8:3: error: ‘b1’ was not declared in this scope; did you mean 
>> ‘a1’?
>> 
>> We can see the problem with g++ -E:
>> 
>> # 1 "a.inline.hpp" 1
>> # 1 "a.hpp" 1
>> 
>> void a1();
>> void a2();
>> 
>> # 4 "a.inline.hpp" 2
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> # 4 "b.inline.hpp" 2
>> # 1 "b.hpp" 1
>> 
>> void b1();
>> void b2();
>> 
>> # 5 "b.inline.hpp" 2
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> # 2 "b.cpp" 2
>> 
>> int main() {
>>   b1();
>> 
>>   return 0;
>> }
>> 
>> 
>> b1() is called before it has been declared. b.inline.hpp inlined 
>> a.inline.hpp, which uses functions declared in b.hpp. However, at that point 
>> we've not included enough of b.inline.hpp to have declared b1().
>> 
>> This is a small and easy example. In the JVM the circular dependencies 
>> usually involves more than two files, and often from different sub-systems 
>> of the JVM. We have so-far solved this by restructuring the code, making 
>> functions out-of-line, creating proxy objects, etc. However, the more we use 
>> templates, the more this is going to become a reoccurring nuisance. And in 
>> experiments with lambdas, which requires templates, this very quickly showed 
>> up as a problem.
>> 
>> I propose that we solve most (all?) of these issues by adding a rule that 
>> states that .inline.hpp files should start by including the corresponding 
>> .hpp, and that the .hpp should contain the declarations of all externally 
>> used parts of the .inline.hpp file. This should guarantee that the 
>> declarations are always present before any subsequent include can create a 
>> circular include dependency back to the original file.
>> 
>> In the example above, b.inline.hpp would become:
>> 
>> // b.inline.hpp
>> #pragma once
>> 
>> #include "b.hpp"
>> #include "a.inline.hpp"
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> and now both a.cpp and b.cpp compiles. The generated output now looks like 
>> this:
>> 
>> # 1 "b.inline.hpp" 1
>> # 1 "b.hpp" 1
>> 
>> void b1();
>> void b2();
>> 
>> # 4 "b.inline.hpp" 2
>> # 1 "a.inline.hpp" 1
>> 
>> # 1 "a.hpp" 1
>> 
>> void a1();
>> void a2();
>> 
>> # 4 "a.inline.hpp" 2
>> 
>> inline void a1() {
>>   b1();
>> }
>> 
>> inline void a2() {
>> }
>> 
>> # 5 "b.inline.hpp" 2
>> 
>> inline void b1() {
>> }
>> 
>> inline void b2() {
>>   a1();
>> }
>> 
>> # 2 "

Re: RFR: 8267464: Circular-dependency resilient inline headers [v6]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Review kbarrett

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4127/files
  - new: https://git.openjdk.java.net/jdk/pull/4127/files/0cf6257d..340bcf49

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4127=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4127=04-05

  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v4]

[Stefan Karlsson]

On Wed, 26 May 2021 02:18:06 GMT, David Holmes  wrote:

> Hi Stefan,
> 
> I think this policy makes a lot of sense!
> 
> I only scanned through the changes to get a sense of them but it all seemed 
> fine.
> 
> Thanks,
> David

Thanks a lot for reviewing!

> doc/hotspot-style.html line 97:
> 
>> 95: Do not put non-trivial function implementations in .hpp files. If 
>> the implementation depends on other .hpp files, put it in a .cpp or a 
>> .inline.hpp file.
>> 96: .inline.hpp files should only be included in .cpp or .inline.hpp 
>> files.
>> 97: All .inline.hpp files should include their corresponding .hpp 
>> file as the first include line. Externally used declarations should be put 
>> in the .hpp file, and not in the the .inline.hpp file. This rule exists to 
>> resolve problems with circular dependencies between.inline.hpp 
>> files.
> 
> typo: the the
> Need space after between on last line

Fixed.

> doc/hotspot-style.md line 142:
> 
>> 140: 
>> 141: * All .inline.hpp files should include their corresponding .hpp file
>> 142: as the first include line. Externally used declarations should be put
> 
> Do we need to spell out it should be first unless precompiled.hpp is included?

We should never include precompiled.hpp from an .inline.hpp file, so I don't 
think we need to state that.

-

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v5]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Review dholmes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4127/files
  - new: https://git.openjdk.java.net/jdk/pull/4127/files/ba9ad1b1..0cf6257d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4127=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4127=03-04

  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v4]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Update HotSpot style guide documents

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4127/files
  - new: https://git.openjdk.java.net/jdk/pull/4127/files/4bcd4348..ba9ad1b1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4127=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4127=02-03

  Stats: 6 lines in 2 files changed: 6 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v3]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request with a new target base due to a 
merge or a rebase. The incremental webrev excludes the unrelated changes 
brought in by the merge/rebase. The pull request contains three additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into 
8267464_circular-dependency_resilient_inline_headers
 - Clean up assembler_.inline.hpp
 - 8267464: Circular-dependency resiliant inline headers

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4127/files
  - new: https://git.openjdk.java.net/jdk/pull/4127/files/260c1115..4bcd4348

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4127=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4127=01-02

  Stats: 5145 lines in 153 files changed: 2603 ins; 1960 del; 582 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v2]

[Stefan Karlsson]

On Mon, 24 May 2021 00:14:06 GMT, Kim Barrett  wrote:

>> Stefan Karlsson has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Clean up assembler_.inline.hpp
>
> Code changes look fine.  This needs an update to the style guide too.

Thanks @kimbarrett and @fisk for reviewing!

-

PR: https://git.openjdk.java.net/jdk/pull/4127

Re: RFR: 8267464: Circular-dependency resilient inline headers [v2]

[Stefan Karlsson]

sed declarations have been placed in .inline.hpp files, 
> and not in the .hpp. Those should be moved. I have not done that.
> 2) Some .inline.hpp files don't have a corresponding .hpp file. I could 
> either try to fix that in this patch, or we could take care of that as 
> separate patches later.
> 3) The style I chose was to add the .hpp include and a extra blank line, 
> separating it from the rest of the includes. I think I like that style, 
> because it makes it easy for those writing .inline.hpp to recognize this 
> pattern. And it removes the confusion why this include isn't sorted into the 
> other includes.
> 4) We have a few *special* platform dependent header files. Both those that 
> simply are included into platform independent files, and those that inject 
> code *into* the platform independent classes. Extra care, as always, need to 
> be taken around those files.
> 5) Mostly tested locally, but I'll test on more platforms if the idea is 
> accepted.
> 
> What do you think?

Stefan Karlsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Clean up assembler_.inline.hpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4127/files
  - new: https://git.openjdk.java.net/jdk/pull/4127/files/6f3fe6af..260c1115

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4127=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4127=00-01

  Stats: 45 lines in 28 files changed: 3 ins; 24 del; 18 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4127.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4127/head:pull/4127

PR: https://git.openjdk.java.net/jdk/pull/4127

RFR: 8267464: Circular-dependency resiliant inline headers

[Stefan Karlsson]

I'd like to propose a small adjustment to how we write .inline.hpp files, in 
the hope to reduce include problems because of circular dependencies between 
inline headers.

This is a small, contrived example to show the problem:

// a.hpp
#pragma once

void a1();
void a2();


// a.inline.hpp
#pragma once

#include "a.hpp"
#include "b.inline.hpp"

inline void a1() {
  b1();
}

inline void a2() {
}


// b.hpp
#pragma once

void b1();
void b2();


// b.inline.hpp
#pragma once

#include "a.inline.hpp"
#include "b.hpp"

inline void b1() {
}

inline void b2() {
  a1();
}

The following code compiles fine:

// a.cpp
#include "a.inline.hpp"

int main() {
  a1();

  return 0;
}

But the following:

// b.cpp
#include "b.inline.hpp"

int main() {
  b1();

  return 0;
}


fails with the this error message:

In file included from b.inline.hpp:3,
 from b.cpp:1:
a.inline.hpp: In function ‘void a1()’:
a.inline.hpp:8:3: error: ‘b1’ was not declared in this scope; did you mean ‘a1’?

We can see the problem with g++ -E:

# 1 "a.inline.hpp" 1
# 1 "a.hpp" 1

void a1();
void a2();

# 4 "a.inline.hpp" 2

inline void a1() {
  b1();
}

inline void a2() {
}

# 4 "b.inline.hpp" 2
# 1 "b.hpp" 1

void b1();
void b2();

# 5 "b.inline.hpp" 2

inline void b1() {
}

inline void b2() {
  a1();
}

# 2 "b.cpp" 2

int main() {
  b1();

  return 0;
}


b1() is called before it has been declared. b.inline.hpp inlined a.inline.hpp, 
which uses functions declared in b.hpp. However, at that point we've not 
included enough of b.inline.hpp to have declared b1().

This is a small and easy example. In the JVM the circular dependencies usually 
involves more than two files, and often from different sub-systems of the JVM. 
We have so-far solved this by restructuring the code, making functions 
out-of-line, creating proxy objects, etc. However, the more we use templates, 
the more this is going to become a reoccurring nuisance. And in experiments 
with lambdas, which requires templates, this very quickly showed up as a 
problem.

I propose that we solve most (all?) of these issues by adding a rule that 
states that .inline.hpp files should start by including the corresponding .hpp, 
and that the .hpp should contain the declarations of all externally used parts 
of the .inline.hpp file. This should guarantee that the declarations are always 
present before any subsequent include can create a circular include dependency 
back to the original file.

In the example above, b.inline.hpp would become:

// b.inline.hpp
#pragma once

#include "b.hpp"
#include "a.inline.hpp"

inline void b1() {
}

inline void b2() {
  a1();
}

and now both a.cpp and b.cpp compiles. The generated output now looks like this:

# 1 "b.inline.hpp" 1
# 1 "b.hpp" 1

void b1();
void b2();

# 4 "b.inline.hpp" 2
# 1 "a.inline.hpp" 1

# 1 "a.hpp" 1

void a1();
void a2();

# 4 "a.inline.hpp" 2

inline void a1() {
  b1();
}

inline void a2() {
}

# 5 "b.inline.hpp" 2

inline void b1() {
}

inline void b2() {
  a1();
}

# 2 "b.cpp" 2

int main() {
  b1();

  return 0;
}

The declarations come first, and the compiler is happy. 

An alternative to this, that has been proposed by other HotSpot GC devs have 
been to always include all .hpp files first, and then have a section of 
.inline.hpp includes. I've experimented with that as well, but I think it 
requires more maintenance and vigilance of  *users* .inline.hpp files (add .hpp 
include to the first section, add .inline.hpp section to second). The proposed 
structures only requires extra care from *writers* of .inline.hpp files. All 
others can include .hpp and .inline.hpp as we've been doing for a long time now.

Some notes about this patch:
1) Some externally-used declarations have been placed in .inline.hpp files, and 
not in the .hpp. Those should be moved. I have not done that.
2) Some .inline.hpp files don't have a corresponding .hpp file. I could either 
try to fix that in this patch, or we could take care of that as separate 
patches later.
3) The style I chose was to add the .hpp include and a extra blank line, 
separating it from the rest of the includes. I think I like that style, because 
it makes it easy for those writing .inline.hpp to recognize this pattern. And 
it removes the confusion why this include isn't sorted into the other includes.
4) We have a few *special* platform dependent header files. Both those that 
simply are included into platform independent files, and those that inject code 
*into* the platform independent classes. Extra care, as always, need to be 
taken around those files.
5) Mostly tested locally, but I'll test on more platforms if the idea is 
accepted.

What do you think?

-

Commit messages:
 - 8267464: Circular-dependency resiliant inline headers

Changes: https://git.openjdk.java.net/jdk/pull/4127/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4127=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267464
  Stats: 459 lines in 224 files changed: 350 ins; 

Re: RFR: 8266531: ZAddress::address() should be removed from SA

[Stefan Karlsson]

On Wed, 5 May 2021 01:35:11 GMT, Yasumasa Suenaga  wrote:

> `ZAddress::address()` has been removed since 
> [JDK-8235748](https://bugs.openjdk.java.net/browse/JDK-8235748), however SA 
> has not followed it. Thus some SA tests would fail with ZGC.
> After this change, more than half of jtreg tests which are listed in 
> ProblemList-zgc.txt pass on Linux x64 with -vmoption:-XX:+UseZGC. Please see 
> the change of ProblemList-zgc.txt what test can be resolved.

The HotSpot changes look good.

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3868

Re: RFR: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods

[Stefan Karlsson]

On Wed, 14 Apr 2021 00:35:55 GMT, Tom Rodriguez  wrote:

> 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of 
> nmethods

src/hotspot/share/prims/jvmtiExport.cpp line :

> 1109:   : JvmtiMethodEventMark(thread,methodHandle(thread, 
> nm->method())) {
> 1110: _code_data = nm->code_begin();
> : _code_size = (jint)pointer_delta(nm->code_end(), nm->code_begin(), 
> sizeof(char));

Could CodeBlob::code_size be used instead?:

  int code_size() const  { return   code_end()  
 -   code_begin();   }

-

PR: https://git.openjdk.java.net/jdk/pull/3481

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Stefan Karlsson]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

GC changes look good.

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264166: OopStorage should support specifying MEMFLAGS for allocations

[Stefan Karlsson]

On Thu, 25 Mar 2021 07:27:58 GMT, Kim Barrett  wrote:

> Please review this change to OopStorage to allow the MEMFLAGS value for 
> associated allocations to be specified when the storage object is 
> constructed.  This allows a subsystem that needs an OopStorage object to 
> associate its allocation with others for that subsystem in NMT tracking and 
> reporting.
> 
> Testing:
> mach5 tier1.
> Manually compared NMT output before and after this change for a test that 
> generated a lot of one particular OopStorage entries.

Looks good. Just one nit

src/hotspot/share/gc/shared/oopStorage.inline.hpp line 28:

> 26: #define SHARE_GC_SHARED_OOPSTORAGE_INLINE_HPP
> 27: 
> 28: #include "memory/allocation.hpp"

Sort order

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3188

Integrated: 8263589: Introduce JavaValue::get_oop/set_oop

[Stefan Karlsson]

On Mon, 15 Mar 2021 12:35:47 GMT, Stefan Karlsson  wrote:

> JavaValue is a small wrapper class that wraps values used to pass arguments 
> and results between native and Java.
> 
> When JavaCalls::call returns an object, the value stored in the JavaValue is 
> not a handliezed jobject. Instead it's a raw oop. So, most of the code 
> handling the `result`, fetches the result as a jobject, and then immediately 
> casts it to an oop. For example:
>   oop res = (oop)result.get_jobject();
> 
> I'd like to change this code to be:
>   oop res = result.get_oop();
> 
> The motivations for this patch is:
> 
> 1) Minimize the places where we pass around oops in jobject variables. Maybe 
> at some point we'll have converted the JVM to only use the jobject type when 
> passing around JNI handle. We need to be stricter with the types when we 
> continue develop our GCs and their barriers.
> 
> 2) Limit the number of places in the code where we perform raw oop casts. We 
> have a helper cast function for that, cast_to_oop, but not all code use it. I 
> have future patches where the compiler will completely forbid raw cast to 
> oops (in fastdebug builds). With that in place, I can then add more stricter 
> oop verification code when oops are created. This helps catching bugs earlier.
> 
> ---
> 
> When reviewing this patch, take an extra look at the change  to 
> oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code:
>   JVMCIObject wrap(oop obj)...
>   JVMCIObjectArray wrap(objArrayOop obj)...
>   JVMCIPrimitiveArray wrap(typeArrayOop obj) ...
> Previously, `wrap((oop)result.get_jobject())` called the first function. When 
> the code was changed to `wrap(result.get_oop())`, where `get_oop()` returns a 
> `oopDesc*`, the compiler didn't know what conversion in oopsHierarchy.hpp to 
> use. Therefore, I replaced the overly permissive `void*` constructor with a 
> constructor that only takes the corresponding `type##OopDesc*`.
> 
> An alternative would be to let get_oop() return an oop, but then that would 
> add an unwanted a dependency between globalDefinitions.hpp and  
> oopsHierarchy.hpp. An earlier version of this patch did return an oop instead 
> of oopDesc*, but it also moved entire JavaValue class out of 
> globalDefinitions.hpp into a new javaValue.hpp file, and had a corresponding 
> javaValue.inline.hpp file.
> 
> Even if we end up using the proposed `oopDesc* get_oop()` version, maybe 
> moving the class to javaValues.hpp would still makes sense?

This pull request has now been integrated.

Changeset: a1f6591f
Author:Stefan Karlsson 
URL:   https://git.openjdk.java.net/jdk/commit/a1f6591f
Stats: 72 lines in 26 files changed: 5 ins; 0 del; 67 mod

8263589: Introduce JavaValue::get_oop/set_oop

Reviewed-by: coleenp, sspitsyn

-

PR: https://git.openjdk.java.net/jdk/pull/3013

Re: RFR: 8263589: Introduce JavaValue::get_oop/set_oop

[Stefan Karlsson]

On Mon, 15 Mar 2021 21:27:54 GMT, Coleen Phillimore  wrote:

>> JavaValue is a small wrapper class that wraps values used to pass arguments 
>> and results between native and Java.
>> 
>> When JavaCalls::call returns an object, the value stored in the JavaValue is 
>> not a handliezed jobject. Instead it's a raw oop. So, most of the code 
>> handling the `result`, fetches the result as a jobject, and then immediately 
>> casts it to an oop. For example:
>>   oop res = (oop)result.get_jobject();
>> 
>> I'd like to change this code to be:
>>   oop res = result.get_oop();
>> 
>> The motivations for this patch is:
>> 
>> 1) Minimize the places where we pass around oops in jobject variables. Maybe 
>> at some point we'll have converted the JVM to only use the jobject type when 
>> passing around JNI handle. We need to be stricter with the types when we 
>> continue develop our GCs and their barriers.
>> 
>> 2) Limit the number of places in the code where we perform raw oop casts. We 
>> have a helper cast function for that, cast_to_oop, but not all code use it. 
>> I have future patches where the compiler will completely forbid raw cast to 
>> oops (in fastdebug builds). With that in place, I can then add more stricter 
>> oop verification code when oops are created. This helps catching bugs 
>> earlier.
>> 
>> ---
>> 
>> When reviewing this patch, take an extra look at the change  to 
>> oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code:
>>   JVMCIObject wrap(oop obj)...
>>   JVMCIObjectArray wrap(objArrayOop obj)...
>>   JVMCIPrimitiveArray wrap(typeArrayOop obj) ...
>> Previously, `wrap((oop)result.get_jobject())` called the first function. 
>> When the code was changed to `wrap(result.get_oop())`, where `get_oop()` 
>> returns a `oopDesc*`, the compiler didn't know what conversion in 
>> oopsHierarchy.hpp to use. Therefore, I replaced the overly permissive 
>> `void*` constructor with a constructor that only takes the corresponding 
>> `type##OopDesc*`.
>> 
>> An alternative would be to let get_oop() return an oop, but then that would 
>> add an unwanted a dependency between globalDefinitions.hpp and  
>> oopsHierarchy.hpp. An earlier version of this patch did return an oop 
>> instead of oopDesc*, but it also moved entire JavaValue class out of 
>> globalDefinitions.hpp into a new javaValue.hpp file, and had a corresponding 
>> javaValue.inline.hpp file.
>> 
>> Even if we end up using the proposed `oopDesc* get_oop()` version, maybe 
>> moving the class to javaValues.hpp would still makes sense?
>
> This change looks really good to me.  I have no objection to oopDesc* in 
> JavaCallValue.  We use oopDesc* in all places where the class oop would 
> interfere with values passed between Java and the vm.

Thanks @coleenp @sspitsyn for reviewing!

-

PR: https://git.openjdk.java.net/jdk/pull/3013

Re: RFR: 8263589: Introduce JavaValue::get_oop/set_oop

[Stefan Karlsson]

On Mon, 15 Mar 2021 21:25:30 GMT, Coleen Phillimore  wrote:

>> JavaValue is a small wrapper class that wraps values used to pass arguments 
>> and results between native and Java.
>> 
>> When JavaCalls::call returns an object, the value stored in the JavaValue is 
>> not a handliezed jobject. Instead it's a raw oop. So, most of the code 
>> handling the `result`, fetches the result as a jobject, and then immediately 
>> casts it to an oop. For example:
>>   oop res = (oop)result.get_jobject();
>> 
>> I'd like to change this code to be:
>>   oop res = result.get_oop();
>> 
>> The motivations for this patch is:
>> 
>> 1) Minimize the places where we pass around oops in jobject variables. Maybe 
>> at some point we'll have converted the JVM to only use the jobject type when 
>> passing around JNI handle. We need to be stricter with the types when we 
>> continue develop our GCs and their barriers.
>> 
>> 2) Limit the number of places in the code where we perform raw oop casts. We 
>> have a helper cast function for that, cast_to_oop, but not all code use it. 
>> I have future patches where the compiler will completely forbid raw cast to 
>> oops (in fastdebug builds). With that in place, I can then add more stricter 
>> oop verification code when oops are created. This helps catching bugs 
>> earlier.
>> 
>> ---
>> 
>> When reviewing this patch, take an extra look at the change  to 
>> oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code:
>>   JVMCIObject wrap(oop obj)...
>>   JVMCIObjectArray wrap(objArrayOop obj)...
>>   JVMCIPrimitiveArray wrap(typeArrayOop obj) ...
>> Previously, `wrap((oop)result.get_jobject())` called the first function. 
>> When the code was changed to `wrap(result.get_oop())`, where `get_oop()` 
>> returns a `oopDesc*`, the compiler didn't know what conversion in 
>> oopsHierarchy.hpp to use. Therefore, I replaced the overly permissive 
>> `void*` constructor with a constructor that only takes the corresponding 
>> `type##OopDesc*`.
>> 
>> An alternative would be to let get_oop() return an oop, but then that would 
>> add an unwanted a dependency between globalDefinitions.hpp and  
>> oopsHierarchy.hpp. An earlier version of this patch did return an oop 
>> instead of oopDesc*, but it also moved entire JavaValue class out of 
>> globalDefinitions.hpp into a new javaValue.hpp file, and had a corresponding 
>> javaValue.inline.hpp file.
>> 
>> Even if we end up using the proposed `oopDesc* get_oop()` version, maybe 
>> moving the class to javaValues.hpp would still makes sense?
>
> src/hotspot/share/utilities/globalDefinitions.hpp line 809:
> 
>> 807: jint i;
>> 808: jlongl;
>> 809: jobject  h;
> 
> Do we still need jobject after this change?

We still use jobject for the arguments. We also converted the result to a 
jobject when it is passed back to Java.

We have a few places in JFR where the code seems to take a detour and fetch the 
oop from the result JavaValue, create a JNI handle, puts it back with 
set_jobject, and then reads it out with get_jobject. I have a patch where the 
code simply returns the created JNI handle without installing it into the 
result JavaValue. I've left that as a separate patch for the JFR team to 
review. 

There are some usages in C1, but I haven't tried to figure that out.

-

PR: https://git.openjdk.java.net/jdk/pull/3013

RFR: 8263589: Introduce JavaValue::get_oop/set_oop

[Stefan Karlsson]

JavaValue is a small wrapper class that wraps values used to pass arguments and 
results between native and Java.

When JavaCalls::call returns an object, the value stored in the JavaValue is 
not a handliezed jobject. Instead it's a raw oop. So, most of the code handling 
the `result`, fetches the result as a jobject, and then immediately casts it to 
an oop. For example:
  oop res = (oop)result.get_jobject();

I'd like to change this code to be:
  oop res = result.get_oop();

The motivations for this patch is:

1) Minimize the places where we pass around oops in jobject variables. Maybe at 
some point we'll have converted the JVM to only use the jobject type when 
passing around JNI handle. We need to be stricter with the types when we 
continue develop our GCs and their barriers.

2) Limit the number of places in the code where we perform raw oop casts. We 
have a helper cast function for that, cast_to_oop, but not all code use it. I 
have future patches where the compiler will completely forbid raw cast to oops 
(in fastdebug builds). With that in place, I can then add more stricter oop 
verification code when oops are created. This helps catching bugs earlier.

---

When reviewing this patch, take an extra look at the change  to 
oopsHierarchy.hpp. This was done to support jvmciEnv.cpp code:
  JVMCIObject wrap(oop obj)...
  JVMCIObjectArray wrap(objArrayOop obj)...
  JVMCIPrimitiveArray wrap(typeArrayOop obj) ...
Previously, `wrap((oop)result.get_jobject())` called the first function. When 
the code was changed to `wrap(result.get_oop())`, where `get_oop()` returns a 
`oopDesc*`, the compiler didn't know what conversion in oopsHierarchy.hpp to 
use. Therefore, I replaced the overly permissive `void*` constructor with a 
constructor that only takes the corresponding `type##OopDesc*`.

An alternative would be to let get_oop() return an oop, but then that would add 
an unwanted a dependency between globalDefinitions.hpp and  oopsHierarchy.hpp. 
An earlier version of this patch did return an oop instead of oopDesc*, but it 
also moved entire JavaValue class out of globalDefinitions.hpp into a new 
javaValue.hpp file, and had a corresponding javaValue.inline.hpp file.

Even if we end up using the proposed `oopDesc* get_oop()` version, maybe moving 
the class to javaValues.hpp would still makes sense?

-

Commit messages:
 - Commit unstaged changes
 - 8263589: Introduce JavaValue::get_oop/set_oop

Changes: https://git.openjdk.java.net/jdk/pull/3013/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3013=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263589
  Stats: 72 lines in 26 files changed: 5 ins; 0 del; 67 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3013.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3013/head:pull/3013

PR: https://git.openjdk.java.net/jdk/pull/3013

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]

[Stefan Karlsson]

On Tue, 9 Mar 2021 17:55:12 GMT, Anton Kozlov  wrote:

>> src/hotspot/share/runtime/thread.cpp line 2515:
>> 
>>> 2513: void JavaThread::check_special_condition_for_native_trans(JavaThread 
>>> *thread) {
>>> 2514:   // Enable WXWrite: called directly from interpreter native wrapper.
>>> 2515:   MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));
>> 
>> FWIW, I personally think that adding these MACOS_AARCH64_ONLY usages at the 
>> call sites increase the line-noise in the affected functions. I think I 
>> would have preferred a version:
>> ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) {
>>   MACOS_AARCH64_ONLY(initialize(new_mode, thread);) {}
>> void initialize(...); // Implementation in thread_bsd_aarch64.cpp (alt. 
>> inline.hpp)
>> With that said, I'm fine with taking this discussion as a follow-up.
>
> The former version used no such macros. I like that now it's clear the W^X 
> management is relevant to macos/aarch64 only. I see the point to move the 
> pre-processor condition into the class implementation. But I think it will 
> bring a bit of inconsistency, as the rest of W^X implementation is explicitly 
> guarded by preprocessor conditionals. I've also tried to push macro 
> conditionals as far as possible down to implementation, providing a kind of 
> generalized W^X interface. That required a few artificial decisions, e.g. how 
> would we call the mode we execute on the rest of platforms with write and 
> execute allowed, WXWriteExec?.. I abandoned that attempt.

I think we would use the same names, but I haven't given it more thought. I 
might take a look at this after this has been integrated.

>> src/hotspot/share/runtime/thread.hpp line 848:
>> 
>>> 846:   void init_wx();
>>> 847:   WXMode enable_wx(WXMode new_state);
>>> 848: #endif // __APPLE__ && AARCH64
>> 
>> Now that this is only compiled into macOS/AArch64, could this be moved over 
>> to thread_bsd_aarch64.hpp? The same goes for the associated functions.
>
> The thread_bsd_aarch64.hpp describes a part of JavaThread, while this block 
> belongs to Thread for now. Since W^X is an attribute of any operating system 
> thread, I assumed Thread to be the right place for W^X bookkeeping. 
> 
> In most cases, we manage W^X state of JavaThread. But sometimes a GC thread 
> needs the WXWrite state, or safefetch is called from non-JavaThread. Probably 
> this can be dealt with (e.g. GCThread to always have the WXWrite state). But 
> such change would be much more than a simple refactoring and it would require 
> a significant amount of testing. Ideally, I would like to investigate this as 
> a follow-up change, or at least after other fixes to this PR.

Good point about Thread vs JavaThread. Yes, this can be looked into as 
follow-up cleanups.

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v25]

[Stefan Karlsson]

On Thu, 11 Mar 2021 14:07:43 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8262903: [macos_aarch64] Thread::current() called on detached thread

Marked as reviewed by stefank (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v12]

[Stefan Karlsson]

On Fri, 5 Feb 2021 16:07:09 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update signal handler part for debugger

Thanks for cleaning out WXWriteFromExecSetter.

src/hotspot/share/gc/shared/barrierSetNMethod.cpp line 52:

> 50: 
> 51: int BarrierSetNMethod::nmethod_stub_entry_barrier(address* 
> return_address_ptr) {
> 52:   // Enable WXWrite: the function is called direclty from 
> nmethod_entry_barrier

direclty -> directly

src/hotspot/share/runtime/threadWXSetters.hpp line 28:

> 26: #define SHARE_RUNTIME_THREADWXSETTERS_HPP
> 27: 
> 28: #include "runtime/thread.inline.hpp"

This breaks against our convention to forbid inline.hpp files from being 
included from being included from .hpp files. You need to rework this by either 
moving the implementation to a .cpp file, or convert this file into an 
.inline.hpp

See the Source Files section in:
https://htmlpreview.github.io/?https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.html

src/hotspot/share/runtime/thread.hpp line 848:

> 846:   void init_wx();
> 847:   WXMode enable_wx(WXMode new_state);
> 848: #endif // __APPLE__ && AARCH64

Now that this is only compiled into macOS/AArch64, could this be moved over to 
thread_bsd_aarch64.hpp? The same goes for the associated functions.

src/hotspot/share/runtime/thread.cpp line 2515:

> 2513: void JavaThread::check_special_condition_for_native_trans(JavaThread 
> *thread) {
> 2514:   // Enable WXWrite: called directly from interpreter native wrapper.
> 2515:   MACOS_AARCH64_ONLY(ThreadWXEnable wx(WXWrite, thread));

FWIW, I personally think that adding these MACOS_AARCH64_ONLY usages at the 
call sites increase the line-noise in the affected functions. I think I would 
have preferred a version:
ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) {
  MACOS_AARCH64_ONLY(initialize(new_mode, thread);) {}
void initialize(...); // Implementation in thread_bsd_aarch64.cpp (alt. 
inline.hpp)
With that said, I'm fine with taking this discussion as a follow-up.

-

Changes requested by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]

[Stefan Karlsson]

On Mon, 25 Jan 2021 19:38:16 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Refactor CDS disabling
>  - Redo builsys support for aarch64-darwin

> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute 
> (W^X), required on macOS/AArch64 platform. It's implemented with 
> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
> thread, so W^X mode change relates to the java thread state change (for java 
> threads). In most cases, JVM executes in write-only mode, except when calling 
> a generated stub like SafeFetch, which requires a temporary switch to 
> execute-only mode. The same execute-only mode is enabled when a java thread 
> executes in java or native states. This approach of managing W^X mode turned 
> out to be simple and efficient enough.

I wonder if this is the right choice. I think it would have been good if this 
could have been completely hidden in the transition classes. However, that 
doesn't seem to have been enough and now there are explicit 
Thread::WXWriteFromExecSetter instances where it's not at all obvious why they 
are needed. For example, why was it added here?:
OopStorageParIterPerf::~OopStorageParIterPerf() {
  // missing transition to vm state
  Thread::WXWriteFromExecSetter wx_write;
  _storage.release(_entries, ARRAY_SIZE(_entries));
}
You need to dig down in the OopStorage implementation to find that it's because 
it uses SafeFetch, which has the opposite transition:
inline int SafeFetch32(int* adr, int errValue) {
  assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");
  Thread::WXExecFromWriteSetter wx_exec;
  return StubRoutines::SafeFetch32_stub()(adr, errValue);
}
I think this adds complexity to code that shouldn't have to deal with this. I'm 
fine with having to force devs / code that writes to exec memory to have to 
deal with a bit more complexity, but it seems wrong to let this leak out to 
code that is staying far away from that.

For my understanding, was this choice made because the nmethods instances (and 
maybe other types as well) are placed in the code cache memory segment, which 
is executable? If the code cache only contained the JIT:ed code, and not object 
instances, I think this could more easily have been contained a bit more.

If the proposed solution is going to stay, maybe this could be made a little 
bit nicer by changing the SafeFetch implementation to use a new scoped object 
that doesn't enforce the "from" state to be "write"? Instead it could check if 
the state is "write" and only then flip the state back and forth. I think, this 
would remove the change needed OopStorageParIterPerf, and probably other places 
as well.

src/hotspot/os/linux/gc/z/zPhysicalMemoryBacking_linux.cpp line 38:

> 36: #include "runtime/os.hpp"
> 37: #include "runtime/stubRoutines.hpp"
> 38: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

src/hotspot/share/runtime/stubRoutines.inline.hpp line 29:

> 27: 
> 28: #include 
> 29: #include 

Replace < > with " "

src/hotspot/os/aix/os_aix.cpp line 70:

> 68: #include "runtime/statSampler.hpp"
> 69: #include "runtime/stubRoutines.hpp"
> 70: #include "runtime/stubRoutines.inline.hpp"

Remove stubRutines.hpp line

-

Changes requested by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2200

Re: RFR: 8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

[Stefan Karlsson]

On Fri, 13 Nov 2020 19:25:57 GMT, Serguei Spitsyn  wrote:

>> The ap01t001 test creates six extra instances of the tested class, let them 
>> die, and then checks that it gets exactly six ObjectFree callbacks. The 
>> problem is that this is verified in the VMDeath callback and at that point 
>> the instance has gone out-of-scope and and a seventh ObjectFree event has 
>> been triggered.
>> 
>> My proposed fix is to ensure that the test instance is kept alive.
>
> Hi Stefan,
> It looks good to me.
> Thank you for fixing it!
> Serguei

Thanks for reviewing!

-

PR: https://git.openjdk.java.net/jdk/pull/1204

Integrated: 8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

[Stefan Karlsson]

On Fri, 13 Nov 2020 14:11:23 GMT, Stefan Karlsson  wrote:

> The ap01t001 test creates six extra instances of the tested class, let them 
> die, and then checks that it gets exactly six ObjectFree callbacks. The 
> problem is that this is verified in the VMDeath callback and at that point 
> the instance has gone out-of-scope and and a seventh ObjectFree event has 
> been triggered.
> 
> My proposed fix is to ensure that the test instance is kept alive.

This pull request has now been integrated.

Changeset: 6a69e304
Author:Stefan Karlsson 
URL:   https://git.openjdk.java.net/jdk/commit/6a69e304
Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod

8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

Reviewed-by: coleenp, sspitsyn

-

PR: https://git.openjdk.java.net/jdk/pull/1204

Re: RFR: 8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

[Stefan Karlsson]

On Fri, 13 Nov 2020 14:11:23 GMT, Stefan Karlsson  wrote:

> The ap01t001 test creates six extra instances of the tested class, let them 
> die, and then checks that it gets exactly six ObjectFree callbacks. The 
> problem is that this is verified in the VMDeath callback and at that point 
> the instance has gone out-of-scope and and a seventh ObjectFree event has 
> been triggered.
> 
> My proposed fix is to ensure that the test instance is kept alive.

I've tested this by running the following reproducer that used to trigger this 
bug:
`while true; do make -C ../build/fastdebug jdk test-only 
TEST=test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP01/ap01t001/TestDescription.java
 JTREG="JAVA_OPTIONS= -XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=0.01"; done`

-

PR: https://git.openjdk.java.net/jdk/pull/1204

RFR: 8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

[Stefan Karlsson]

The ap01t001 test creates six extra instances of the tested class, let them 
die, and then checks that it gets exactly six ObjectFree callbacks. The problem 
is that this is verified in the VMDeath callback and at that point the instance 
has gone out-of-scope and and a seventh ObjectFree event has been triggered.

My proposed fix is to ensure that the test instance is kept alive.

-

Commit messages:
 - 8256337: ap01t001.cpp, 67: Received unexpected number of ObjectFree events: 7

Changes: https://git.openjdk.java.net/jdk/pull/1204/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1204=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256337
  Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1204.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1204/head:pull/1204

PR: https://git.openjdk.java.net/jdk/pull/1204

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Stefan Karlsson]

On Mon, 2 Nov 2020 15:58:15 GMT, Coleen Phillimore  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> Coleen Phillimore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comments from StefanK.

Some more nit-picking to make the code more consistent.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 52:

> 50:   : Hashtable(_table_size, 
> sizeof(JvmtiTagMapEntry)) {}
> 51: 
> 52: 

Double whitespace

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 185:

> 183: // Serially remove unused oops from the table, and notify jvmti.
> 184: void JvmtiTagMapTable::unlink_and_post(JvmtiEnv* env) {
> 185: 

Stray newline

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 224:

> 222: // Rehash oops in the table
> 223: void JvmtiTagMapTable::rehash() {
> 224: 

Stray newline

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 75:

> 73: 
> 74:   void resize_if_needed();
> 75: public:

Newline between

src/hotspot/share/prims/jvmtiTagMapTable.hpp line 100:

> 98: };
> 99: 
> 100: 

Double newline

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 258:

> 256:   int rehash_len = moved_entries.length();
> 257:   // Now add back in the entries that were removed.
> 258:   for (int i = 0; i < moved_entries.length(); i++) {

rehash_len is read, but not used in for loop condition.

src/hotspot/share/prims/jvmtiTagMapTable.cpp line 165:

> 163:   }
> 164: }
> 165: const int _resize_load_trigger = 5;   // load factor that will 
> trigger the resize

Newline between

-

Marked as reviewed by stefank (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table concurrent [v2]

[Stefan Karlsson]

On Mon, 2 Nov 2020 12:58:49 GMT, Coleen Phillimore  wrote:

>  GC callers shouldn't really have to know what processing we're doing here.

I completely disagree with this. It's extremely important that the GC and 
Runtime code agrees on what this code does and where the GC *must* call it. 
Knowing the details allows us to skip calling this after mark end, but forces 
us to call it in relocate start, when objects should start to move. Though, I 
don't want to block this review because of this point, so if you still thinks 
that a non-descriptive name is better then we can argue that separately.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address

[Stefan Karlsson]

On Mon, 2 Nov 2020 08:25:28 GMT, Stefan Karlsson  wrote:

>> This change turns the HashTable that JVMTI uses for object tagging into a 
>> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
>> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
>> table to follow oops and then to rehash the table, this table points to 
>> WeakHandle.  GC walks the backing OopStorages concurrently.
>> 
>> The hash function for the table is a hash of the lower 32 bits of the 
>> address.  A flag is set during GC (gc_notification if in a safepoint, and 
>> through a call to JvmtiTagMap::needs_processing()) so that the table is 
>> rehashed at the next use.
>> 
>> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
>> to post ObjectFree events.  In concurrent GCs there can be a window of time 
>> between weak oop marking where the oop is unmarked, so dead (the phantom 
>> load in peek returns NULL) but the gc_notification hasn't been done yet.  In 
>> this window, a heap walk or GetObjectsWithTags call would not find an object 
>> before the ObjectFree event is posted.  This is dealt with in two ways:
>> 
>> 1. In the Heap walk, there's an unconditional table walk to post events if 
>> events are needed to post.
>> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
>> required, we use the VM thread to post the event.
>> 
>> Event posting cannot be done in a JavaThread because the posting needs to be 
>> done while holding the table lock, so that the JvmtiEnv state doesn't change 
>> before posting is done.  ObjectFree callbacks are limited in what they can 
>> do as per the JVMTI Specification.  The allowed callbacks to the VM already 
>> have code to allow NonJava threads.
>> 
>> To avoid rehashing, I also tried to use object->identity_hash() but this 
>> breaks because entries can be added to the table during heapwalk, where the 
>> objects use marking.  The starting markWord is saved and restored.  Adding a 
>> hashcode during this operation makes restoring the former markWord (locked, 
>> inflated, etc) too complicated.  Plus we don't want all these objects to 
>> have hashcodes because locking operations after tagging would have to always 
>> use inflated locks.
>> 
>> Much of this change is to remove serial weak oop processing for the 
>> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
>> jvmti code.
>> 
>> It has also been tested with tier1-6.
>> 
>> Thank you to Stefan, Erik and Kim for their help with this change.
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 126:
> 
>> 124:   // concurrent GCs. So fix it here once we have a lock or are
>> 125:   // at a safepoint.
>> 126:   // SetTag and GetTag should not post events!
> 
> I think it would be good to explain why. Otherwise, this just leaves the 
> readers wondering why this is the case.

Maybe even move this comment to the set_tag/get_tag code.

-

PR: https://git.openjdk.java.net/jdk/pull/967

Re: RFR: 8212879: Make JVMTI TagMap table not hash on oop address

[Stefan Karlsson]

On Fri, 30 Oct 2020 20:23:04 GMT, Coleen Phillimore  wrote:

> This change turns the HashTable that JVMTI uses for object tagging into a 
> regular Hotspot hashtable - the one in hashtable.hpp with resizing and 
> rehashing.   Instead of pointing directly to oops so that GC has to walk the 
> table to follow oops and then to rehash the table, this table points to 
> WeakHandle.  GC walks the backing OopStorages concurrently.
> 
> The hash function for the table is a hash of the lower 32 bits of the 
> address.  A flag is set during GC (gc_notification if in a safepoint, and 
> through a call to JvmtiTagMap::needs_processing()) so that the table is 
> rehashed at the next use.
> 
> The gc_notification mechanism of weak oop processing is used to notify Jvmti 
> to post ObjectFree events.  In concurrent GCs there can be a window of time 
> between weak oop marking where the oop is unmarked, so dead (the phantom load 
> in peek returns NULL) but the gc_notification hasn't been done yet.  In this 
> window, a heap walk or GetObjectsWithTags call would not find an object 
> before the ObjectFree event is posted.  This is dealt with in two ways:
> 
> 1. In the Heap walk, there's an unconditional table walk to post events if 
> events are needed to post.
> 2. For GetObjectWithTags, if a dead oop is found in the table and posting is 
> required, we use the VM thread to post the event.
> 
> Event posting cannot be done in a JavaThread because the posting needs to be 
> done while holding the table lock, so that the JvmtiEnv state doesn't change 
> before posting is done.  ObjectFree callbacks are limited in what they can do 
> as per the JVMTI Specification.  The allowed callbacks to the VM already have 
> code to allow NonJava threads.
> 
> To avoid rehashing, I also tried to use object->identity_hash() but this 
> breaks because entries can be added to the table during heapwalk, where the 
> objects use marking.  The starting markWord is saved and restored.  Adding a 
> hashcode during this operation makes restoring the former markWord (locked, 
> inflated, etc) too complicated.  Plus we don't want all these objects to have 
> hashcodes because locking operations after tagging would have to always use 
> inflated locks.
> 
> Much of this change is to remove serial weak oop processing for the 
> weakProcessor, ZGC and Shenandoah.  The GCs have been stress tested with 
> jvmti code.
> 
> It has also been tested with tier1-6.
> 
> Thank you to Stefan, Erik and Kim for their help with this change.

Commented on nits, and reviewed GC code and tag map code. Didn't look closely 
on the hashmap changes.

src/hotspot/share/gc/shared/oopStorageSet.hpp line 41:

> 39:   // Must be updated when new OopStorages are introduced
> 40:   static const uint strong_count = 4 JVMTI_ONLY(+ 1);
> 41:   static const uint weak_count = 5 JVMTI_ONLY(+1) JFR_ONLY(+ 1);

All other uses `+ 1` instead of `+1`.

src/hotspot/share/gc/shared/weakProcessorPhaseTimes.hpp line 49:

> 47:   double _phase_times_sec[1];
> 48:   size_t _phase_dead_items[1];
> 49:   size_t _phase_total_items[1];

This should be removed and the associated reset_items

src/hotspot/share/gc/z/zOopClosures.hpp line 64:

> 62: };
> 63: 
> 64: class ZPhantomKeepAliveOopClosure : public ZRootsIteratorClosure {

Seems like you flipped the location of these two. Maybe revert?

src/hotspot/share/prims/jvmtiExport.hpp line 405:

> 403: 
> 404:   // Delete me and all my callers!
> 405:   static void weak_oops_do(BoolObjectClosure* b, OopClosure* f) {}

Maybe delete?

src/hotspot/share/prims/jvmtiTagMap.cpp line 126:

> 124:   // concurrent GCs. So fix it here once we have a lock or are
> 125:   // at a safepoint.
> 126:   // SetTag and GetTag should not post events!

I think it would be good to explain why. Otherwise, this just leaves the 
readers wondering why this is the case.

src/hotspot/share/prims/jvmtiTagMap.cpp line 131:

> 129:   // Operating on the hashmap must always be locked, since concurrent GC 
> threads may
> 130:   // notify while running through a safepoint.
> 131:   assert(is_locked(), "checking");

Maybe move this to the top of the function to make it very clear.

src/hotspot/share/prims/jvmtiTagMap.cpp line 133:

> 131:   assert(is_locked(), "checking");
> 132:   if (post_events && env()->is_enabled(JVMTI_EVENT_OBJECT_FREE)) {
> 133: log_info(jvmti, table)("TagMap table needs posting before heap 
> walk");

Not sure about the "before heap walk" since this is also done from 
GetObjectsWithTags, which does *not* do a heap walk but still requires posting.

src/hotspot/share/prims/jvmtiTagMap.cpp line 140:

> 138: hashmap()->rehash();
> 139: _needs_rehashing = false;
> 140:   }

It's not clear to me that it's correct to rehash *after* posting. I think it 
is, because unlink_and_post will use load barriers to fixup old pointers.

src/hotspot/share/prims/jvmtiTagMap.cpp line 146:

> 144: // The ZDriver may be walking the hashmaps concurrently so all these 

Re: RFR: 8252103: Parallel heap inspection for ParallelScavengeHeap

[Stefan Karlsson]

On Mon, 19 Oct 2020 12:21:27 GMT, Lin Zang  wrote:

> Dear @stefank,
> I have update this PR that use a claimer to help worker thread do parallel 
> iteration. would you like to help review
> again? Thanks,
> Lin

Wrong Stefan, I think you mean @kstefanj

-

PR: https://git.openjdk.java.net/jdk/pull/25

Integrated: 8254874: ZGC: JNIHandleBlock verification failure in stack watermark processing

[Stefan Karlsson]

On Fri, 16 Oct 2020 14:29:46 GMT, Stefan Karlsson  wrote:

> The cm03t001 test creates a local JNI handle in the prepare function. It 
> later uses that handle from a callback
> function, from another thread. When the callback runs, ZGC applies a load 
> barrier to that handle and self-heals it in
> the other threads stack. Later when that thread verifies its stack, during 
> the start of its stack processing, it finds
> that the oop is unexpectedly not "bad".  It's invalid to send a local JNI 
> handle over to another thread:
> https://docs.oracle.com/en/java/javase/15/docs/specs/jni/design.html#global-and-local-references
> So, my proposed fix is to convert the local handle to a global handle.
> 
> I've tested this with the reproducer in the bug report.

This pull request has now been integrated.

Changeset: 672f5669
Author:Stefan Karlsson 
URL:   https://git.openjdk.java.net/jdk/commit/672f5669
Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod

8254874: ZGC: JNIHandleBlock verification failure in stack watermark processing

Reviewed-by: tschatzl, cjplummer, sspitsyn, pliden

-

PR: https://git.openjdk.java.net/jdk/pull/701

Re: RFR: 8254874: ZGC: JNIHandleBlock verification failure in stack watermark processing

[Stefan Karlsson]

On Sat, 17 Oct 2020 08:38:10 GMT, Per Liden  wrote:

>> The cm03t001 test creates a local JNI handle in the prepare function. It 
>> later uses that handle from a callback
>> function, from another thread. When the callback runs, ZGC applies a load 
>> barrier to that handle and self-heals it in
>> the other threads stack. Later when that thread verifies its stack, during 
>> the start of its stack processing, it finds
>> that the oop is unexpectedly not "bad".  It's invalid to send a local JNI 
>> handle over to another thread:
>> https://docs.oracle.com/en/java/javase/15/docs/specs/jni/design.html#global-and-local-references
>> So, my proposed fix is to convert the local handle to a global handle.
>> 
>> I've tested this with the reproducer in the bug report.
>
> Marked as reviewed by pliden (Reviewer).

Thanks for reviewing!

-

PR: https://git.openjdk.java.net/jdk/pull/701

RFR: 8254874: ZGC: JNIHandleBlock verification failure in stack watermark processing

[Stefan Karlsson]

The cm03t001 test creates a local JNI handle in the prepare function. It later 
uses that handle from a callback
function, from another thread. When the callback runs, ZGC applies a load 
barrier to that handle and self-heals it in
the other threads stack. Later when that thread verifies its stack, during the 
start of its stack processing, it finds
that the oop is unexpectedly not "bad".

It's invalid to send a local JNI handle over to another thread:
https://docs.oracle.com/en/java/javase/15/docs/specs/jni/design.html#global-and-local-references

So, my proposed fix is to convert the local handle to a global handle.

I've tested this with the reproducer in the bug report.

-

Commit messages:
 - 8254874: ZGC: JNIHandleBlock verification failure in stack watermark 
processing

Changes: https://git.openjdk.java.net/jdk/pull/701/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=701=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8254874
  Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/701.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/701/head:pull/701

PR: https://git.openjdk.java.net/jdk/pull/701

Integrated: 8254668: JVMTI process frames on thread without started processing

[Stefan Karlsson]

On Tue, 13 Oct 2020 09:25:55 GMT, Stefan Karlsson  wrote:

> I hit the following assert in some tests runs that I've been doing:
> # Internal Error 
> (/home/stefank/git/alt/open/src/hotspot/share/runtime/stackWatermark.inline.hpp:67),
>  pid=828170,
> tid=828734 # assert(processing_started()) failed: Processing should already 
> have started
> 
> The stack traces for this has been:
> Native frames: (J=compiled Java code, A=aot compiled Java code, 
> j=interpreted, Vv=VM code, C=native code)
> V [libjvm.so+0x1626d75] StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V [libjvm.so+0xad791a] frame::sender(RegisterMap*) const+0x7a
> V [libjvm.so+0xacd3f8] frame::real_sender(RegisterMap*) const+0x18
> V [libjvm.so+0x1804c4a] vframe::sender() const+0xea
> V [libjvm.so+0x175f47b] JavaThread::last_java_vframe(RegisterMap*)+0x5b
> V [libjvm.so+0x10e10fc] JvmtiEnvBase::vframeFor(JavaThread*, int)+0x4c
> V [libjvm.so+0x10e6972] JvmtiEnvBase::check_top_frame(Thread*, JavaThread*, 
> jvalue, TosState, Handle*)+0xe2
> V [libjvm.so+0x10e759c] JvmtiEnvBase::force_early_return(JavaThread*, jvalue, 
> TosState)+0x11c
> V [libjvm.so+0x105b8f5] jvmti_ForceEarlyReturnObject+0x215
> V  [libjvm.so+0x1626d75]  StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V  [libjvm.so+0xad791a]  frame::sender(RegisterMap*) const+0x7a
> V  [libjvm.so+0xacd3f8]  frame::real_sender(RegisterMap*) const+0x18
> V  [libjvm.so+0x1804c4a]  vframe::sender() const+0xea
> V  [libjvm.so+0x1804d00]  vframe::java_sender() const+0x10
> V  [libjvm.so+0x10e1115]  JvmtiEnvBase::vframeFor(JavaThread*, int)+0x65
> V  [libjvm.so+0x10d475f]  JvmtiEnv::NotifyFramePop(JavaThread*, int)+0x9f
> V  [libjvm.so+0x106b6aa]  jvmti_NotifyFramePop+0x23a
> The code inspects the top frame of a suspended java thread. However, there's 
> nothing in the code that starts the
> watermark processing of the thread, so the code asserts when sender calls 
> on_iteration.
> We only have to call start_processing/on_iteration when oops are being read. 
> The failing code does *not* inspect any
> oops, so I turn of the on_iteration call by settings process_frame to false.
> To notify the readers of the code that vframeFor doesn't process the oops, 
> I've renamed the function to
> vframeForNoProcess to give a visual cue.
> I found this bug when running this command line:
> makec ../build/fastdebug/ test TEST=test/hotspot/jtreg/vmTestbase/nsk/jvmti
> JTREG="JAVA_OPTIONS=-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=1 
> -XX:ZFragmentationLimit=0.01"
> JTREG_EXTRA_PROBLEM_LISTS=ProblemList-zgc.txt   Five tests consistently 
> asserts with this command line. All tests pass
> with the proposed fix.
> Recommendations of tests to run are welcome. I intend to get this run through 
> tier1-3, but haven't yet.

This pull request has now been integrated.

Changeset: db9dcdf1
Author:Stefan Karlsson 
URL:   https://git.openjdk.java.net/jdk/commit/db9dcdf1
Stats: 9 lines in 3 files changed: 1 ins; 0 del; 8 mod

8254668: JVMTI process frames on thread without started processing

Reviewed-by: eosterlund, rrich

-

PR: https://git.openjdk.java.net/jdk/pull/627

Re: RFR: 8254668: JVMTI process frames on thread without started processing [v2]

[Stefan Karlsson]

> I hit the following assert in some tests runs that I've been doing:
> # Internal Error 
> (/home/stefank/git/alt/open/src/hotspot/share/runtime/stackWatermark.inline.hpp:67),
>  pid=828170,
> tid=828734 # assert(processing_started()) failed: Processing should already 
> have started
> 
> The stack traces for this has been:
> Native frames: (J=compiled Java code, A=aot compiled Java code, 
> j=interpreted, Vv=VM code, C=native code)
> V [libjvm.so+0x1626d75] StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V [libjvm.so+0xad791a] frame::sender(RegisterMap*) const+0x7a
> V [libjvm.so+0xacd3f8] frame::real_sender(RegisterMap*) const+0x18
> V [libjvm.so+0x1804c4a] vframe::sender() const+0xea
> V [libjvm.so+0x175f47b] JavaThread::last_java_vframe(RegisterMap*)+0x5b
> V [libjvm.so+0x10e10fc] JvmtiEnvBase::vframeFor(JavaThread*, int)+0x4c
> V [libjvm.so+0x10e6972] JvmtiEnvBase::check_top_frame(Thread*, JavaThread*, 
> jvalue, TosState, Handle*)+0xe2
> V [libjvm.so+0x10e759c] JvmtiEnvBase::force_early_return(JavaThread*, jvalue, 
> TosState)+0x11c
> V [libjvm.so+0x105b8f5] jvmti_ForceEarlyReturnObject+0x215
> V  [libjvm.so+0x1626d75]  StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V  [libjvm.so+0xad791a]  frame::sender(RegisterMap*) const+0x7a
> V  [libjvm.so+0xacd3f8]  frame::real_sender(RegisterMap*) const+0x18
> V  [libjvm.so+0x1804c4a]  vframe::sender() const+0xea
> V  [libjvm.so+0x1804d00]  vframe::java_sender() const+0x10
> V  [libjvm.so+0x10e1115]  JvmtiEnvBase::vframeFor(JavaThread*, int)+0x65
> V  [libjvm.so+0x10d475f]  JvmtiEnv::NotifyFramePop(JavaThread*, int)+0x9f
> V  [libjvm.so+0x106b6aa]  jvmti_NotifyFramePop+0x23a
> The code inspects the top frame of a suspended java thread. However, there's 
> nothing in the code that starts the
> watermark processing of the thread, so the code asserts when sender calls 
> on_iteration.
> We only have to call start_processing/on_iteration when oops are being read. 
> The failing code does *not* inspect any
> oops, so I turn of the on_iteration call by settings process_frame to false.
> To notify the readers of the code that vframeFor doesn't process the oops, 
> I've renamed the function to
> vframeForNoProcess to give a visual cue.
> I found this bug when running this command line:
> makec ../build/fastdebug/ test TEST=test/hotspot/jtreg/vmTestbase/nsk/jvmti
> JTREG="JAVA_OPTIONS=-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=1 
> -XX:ZFragmentationLimit=0.01"
> JTREG_EXTRA_PROBLEM_LISTS=ProblemList-zgc.txt   Five tests consistently 
> asserts with this command line. All tests pass
> with the proposed fix.
> Recommendations of tests to run are welcome. I intend to get this run through 
> tier1-3, but haven't yet.

Stefan Karlsson has updated the pull request incrementally with one additional 
commit since the last revision:

  Review 1

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/627/files
  - new: https://git.openjdk.java.net/jdk/pull/627/files/587fd354..00c1b25f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=627=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=627=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/627.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/627/head:pull/627

PR: https://git.openjdk.java.net/jdk/pull/627

Re: RFR: 8254668: JVMTI process frames on thread without started processing [v2]

[Stefan Karlsson]

On Tue, 13 Oct 2020 12:48:05 GMT, Richard Reingruber  wrote:

>> Stefan Karlsson has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Review 1
>
> Hi Stefan,
> 
> thanks for fixing.
> 
> With this change the assertion in pr #119 does not fail anymore.
> 
> The fix looks good to me but I'm not an ZGC expert, neither a Reviewer :)

Thanks @fisk and @reinrich for reviewing.

> src/hotspot/share/prims/jvmtiEnvBase.cpp line 559:
> 
>> 557:
>> 558: // return the vframe on the specified thread and depth, NULL if no such 
>> frame
>> 559: // The thread and the oops in the returned might not have been process.
> 
> s/the returned/the returned vframe/

Thanks for noticing.

-

PR: https://git.openjdk.java.net/jdk/pull/627

Re: RFR: 8254668: JVMTI process frames on thread without started processing

[Stefan Karlsson]

On Tue, 13 Oct 2020 09:25:55 GMT, Stefan Karlsson  wrote:

> I hit the following assert in some tests runs that I've been doing:
> # Internal Error 
> (/home/stefank/git/alt/open/src/hotspot/share/runtime/stackWatermark.inline.hpp:67),
>  pid=828170,
> tid=828734 # assert(processing_started()) failed: Processing should already 
> have started
> 
> The stack traces for this has been:
> Native frames: (J=compiled Java code, A=aot compiled Java code, 
> j=interpreted, Vv=VM code, C=native code)
> V [libjvm.so+0x1626d75] StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V [libjvm.so+0xad791a] frame::sender(RegisterMap*) const+0x7a
> V [libjvm.so+0xacd3f8] frame::real_sender(RegisterMap*) const+0x18
> V [libjvm.so+0x1804c4a] vframe::sender() const+0xea
> V [libjvm.so+0x175f47b] JavaThread::last_java_vframe(RegisterMap*)+0x5b
> V [libjvm.so+0x10e10fc] JvmtiEnvBase::vframeFor(JavaThread*, int)+0x4c
> V [libjvm.so+0x10e6972] JvmtiEnvBase::check_top_frame(Thread*, JavaThread*, 
> jvalue, TosState, Handle*)+0xe2
> V [libjvm.so+0x10e759c] JvmtiEnvBase::force_early_return(JavaThread*, jvalue, 
> TosState)+0x11c
> V [libjvm.so+0x105b8f5] jvmti_ForceEarlyReturnObject+0x215
> V  [libjvm.so+0x1626d75]  StackWatermarkSet::on_iteration(JavaThread*, frame 
> const&)+0xd5
> V  [libjvm.so+0xad791a]  frame::sender(RegisterMap*) const+0x7a
> V  [libjvm.so+0xacd3f8]  frame::real_sender(RegisterMap*) const+0x18
> V  [libjvm.so+0x1804c4a]  vframe::sender() const+0xea
> V  [libjvm.so+0x1804d00]  vframe::java_sender() const+0x10
> V  [libjvm.so+0x10e1115]  JvmtiEnvBase::vframeFor(JavaThread*, int)+0x65
> V  [libjvm.so+0x10d475f]  JvmtiEnv::NotifyFramePop(JavaThread*, int)+0x9f
> V  [libjvm.so+0x106b6aa]  jvmti_NotifyFramePop+0x23a
> The code inspects the top frame of a suspended java thread. However, there's 
> nothing in the code that starts the
> watermark processing of the thread, so the code asserts when sender calls 
> on_iteration.
> We only have to call start_processing/on_iteration when oops are being read. 
> The failing code does *not* inspect any
> oops, so I turn of the on_iteration call by settings process_frame to false.
> To notify the readers of the code that vframeFor doesn't process the oops, 
> I've renamed the function to
> vframeForNoProcess to give a visual cue.
> I found this bug when running this command line:
> makec ../build/fastdebug/ test TEST=test/hotspot/jtreg/vmTestbase/nsk/jvmti
> JTREG="JAVA_OPTIONS=-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=1 
> -XX:ZFragmentationLimit=0.01"
> JTREG_EXTRA_PROBLEM_LISTS=ProblemList-zgc.txt   Five tests consistently 
> asserts with this command line. All tests pass
> with the proposed fix.
> Recommendations of tests to run are welcome. I intend to get this run through 
> tier1-3, but haven't yet.

Notifying @reinrich @fisk since I think they have been looking into similar 
problems.

-

PR: https://git.openjdk.java.net/jdk/pull/627

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing [v7]

[Stefan Karlsson]

On Tue, 29 Sep 2020 06:42:54 GMT, Erik Österlund  wrote:

>> I think the previous assert was intentionally weaker: `is_in` checks that 
>> argument points to a committed area in the
>> heap, which can include the oops not yet fixed. It does not check if oop is 
>> valid as far as GC is concerned. So maybe
>> reverting `NativeAccess::oop_load` is enough?
>
> It was checking is_in_or_null before and after. Our is_in checks that it is 
> in committed memory, and that the pointer
> is not stale w.r.t. color, as that is very likely to be a bug. We reluctantly 
> made it relaxed for our safepoints that
> flip colors. I think it was once again for this very same usual suspect 
> assert. But now that this is concurrent, its
> memory can get concurrently freed.  I think this is the 7th time I try to 
> make this assert happy. I run into it all the
> time. IMO, the underlying assumption of the assert, is wrong. This is an 
> iterator used by GC to fix totally invalid
> stale pointers into valid pointers. You really can't expect that they are 
> valid before fixing them. That just happened
> to be true for other GCs. It makes little sense to me. That is why my 
> preferred solution is to remove the assert. I
> would not miss it.  We have had similar issues with the oop_iterate framework 
> asserting oops are valid before the
> closure is applied. At least there, it is possible to opt out...  Would 
> anyone miss the assert?

FTR, this is the RFE to remove the oop verification from the oop_iterate 
framework:
https://bugs.openjdk.java.net/browse/JDK-8237363

I really would like to get rid of it, but got push back because it made GCs 
have to duplicate the effort to provide
verification.

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing [v3]

[Stefan Karlsson]

On Wed, 23 Sep 2020 13:57:11 GMT, Erik Österlund  wrote:

>> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack 
>> Processing" (cf.
>> https://openjdk.java.net/jeps/376).
>> Basically, this patch modifies the epilog safepoint when returning from a 
>> frame (supporting interpreter frames, c1, c2,
>> and native wrapper frames), to compare the stack pointer against a 
>> thread-local value. This turns return polls into
>> more of a swiss army knife that can be used to poll for safepoints, 
>> handshakes, but also returns into not yet safe to
>> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and 
>> other thread oops) in a state of a mess in
>> the GC checkpoint safepoints, rather than processing all threads and their 
>> stacks. Processing is initialized
>> automagically when threads wake up for a safepoint, or get poked by a 
>> handshake or safepoint. Said initialization
>> processes a few (3) frames and other thread oops. The rest - the bulk of the 
>> frame processing, is deferred until it is
>> actually needed. It is needed when a frame is exposed to either 1) execution 
>> (returns or unwinding due to exception
>> handling), or 2) stack walker APIs. A hook is then run to go and finish the 
>> lazy processing of frames.  Mutator and GC
>> threads can compete for processing. The processing is therefore performed 
>> under a per-thread lock. Note that disarming
>> of the poll word (that the returns are comparing against) is only performed 
>> by the thread itself. So sliding the
>> watermark up will require one runtime call for a thread to note that nothing 
>> needs to be done, and then update the poll
>> word accordingly. Downgrading the poll word concurrently by other threads 
>> was simply not worth the complexity it
>> brought (and is only possible on TSO machines). So left that one out.
>
> Erik Österlund has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review: SteafanK CR 2

Marked as reviewed by stefank (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

[Stefan Karlsson]

On Wed, 23 Sep 2020 12:50:51 GMT, Erik Österlund  wrote:

>> src/hotspot/share/gc/z/zDriver.cpp line 108:
>> 
>>> 106: return false;
>>> 107:   }
>>> 108:
>> 
>> Group needs_inactive_gc_locker, skip_thread_oop_barriers, and 
>> allow_coalesced_vm_operations together?
>> 
>> Add a comment about why we chose to skip coalescing here.
>
> Per explicitly wanted skip_thread_oop_barriers grouped with 
> needs_inactive_gc_locker. But I should remove
> allow_coalesced_vm_operations; it is no longer used since I have removed VM 
> operation coalescing completely already.

I agree with Per, but I also wanted to move allow_coalesced_vm_operations. But 
now your removing it, so it becomes a
moot point.

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

[Stefan Karlsson]

On Tue, 22 Sep 2020 11:43:41 GMT, Erik Österlund  wrote:

> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack 
> Processing" (cf.
> https://openjdk.java.net/jeps/376).
> Basically, this patch modifies the epilog safepoint when returning from a 
> frame (supporting interpreter frames, c1, c2,
> and native wrapper frames), to compare the stack pointer against a 
> thread-local value. This turns return polls into
> more of a swiss army knife that can be used to poll for safepoints, 
> handshakes, but also returns into not yet safe to
> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and 
> other thread oops) in a state of a mess in
> the GC checkpoint safepoints, rather than processing all threads and their 
> stacks. Processing is initialized
> automagically when threads wake up for a safepoint, or get poked by a 
> handshake or safepoint. Said initialization
> processes a few (3) frames and other thread oops. The rest - the bulk of the 
> frame processing, is deferred until it is
> actually needed. It is needed when a frame is exposed to either 1) execution 
> (returns or unwinding due to exception
> handling), or 2) stack walker APIs. A hook is then run to go and finish the 
> lazy processing of frames.  Mutator and GC
> threads can compete for processing. The processing is therefore performed 
> under a per-thread lock. Note that disarming
> of the poll word (that the returns are comparing against) is only performed 
> by the thread itself. So sliding the
> watermark up will require one runtime call for a thread to note that nothing 
> needs to be done, and then update the poll
> word accordingly. Downgrading the poll word concurrently by other threads was 
> simply not worth the complexity it
> brought (and is only possible on TSO machines). So left that one out.

I've gone over the entire patch, but I'm leaving the compiler parts to others 
to review.

src/hotspot/share/jfr/leakprofiler/checkpoint/rootResolver.cpp line 275:

> 273:
> 274: // Traverse the execution stack
> 275: for (StackFrameStream fst(jt, true /* update */, true /* 
> process_frames */); !fst.is_done(); fst.next()) {

Noticed that lines 261-262 refers to the array that was removed with:
JDK-8252656: Replace RegisterArrayForGC mechanism with plain Handles

And the comment in block 299-304 might need some love.

It's not directly related to this patch, but something that has been moved 
around in preparation for this patch. I
wouldn't be opposed to cleaning that up with this patch.

src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp line 39:

> 37: #include "runtime/os.hpp"
> 38: #include "runtime/semaphore.hpp"
> 39: #include "runtime/stackWatermarkSet.hpp"

Revert?

src/hotspot/share/runtime/safepoint.cpp line 507:

> 505:   return;
> 506: }
> 507: 
> StackWatermarkSet::start_processing(static_cast(thread), 
> StackWatermarkKind::gc);

Maybe simply:
if (thread->is_Java_thread()) {
  StackWatermarkSet::start_processing(static_cast(thread), 
StackWatermarkKind::gc);
}

src/hotspot/share/runtime/stackWatermarkSet.cpp line 70:

> 68:   Thread* thread = Thread::current();
> 69:   if (thread->is_Java_thread()) {
> 70: JavaThread* jt = static_cast(thread);

as_Java_thread()

src/hotspot/share/runtime/stackWatermarkSet.cpp line 112:

> 110: return;
> 111:   }
> 112:   verify_poll_context();

There's a verfy_poll_context here, but no update_poll_values call. I guess this 
is because we could be iterating over a
thread that is not Thread::current()? But in that case, should we really be 
"verifying the poll context" of the current
thread?

src/hotspot/share/runtime/stackWatermarkSet.cpp line 125:

> 123: watermark->start_processing();
> 124:   }
> 125: }

No update poll values here?

src/hotspot/share/utilities/vmError.cpp line 754:

> 752:Thread* thread = ((NamedThread *)_thread)->processed_thread();
> 753:if (thread != NULL && thread->is_Java_thread()) {
> 754:  JavaThread* jt = static_cast(thread);

as_Java_thread() - maybe just search for this in the patch

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

[Stefan Karlsson]

On Tue, 22 Sep 2020 11:43:41 GMT, Erik Österlund  wrote:

> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack 
> Processing" (cf.
> https://openjdk.java.net/jeps/376).
> Basically, this patch modifies the epilog safepoint when returning from a 
> frame (supporting interpreter frames, c1, c2,
> and native wrapper frames), to compare the stack pointer against a 
> thread-local value. This turns return polls into
> more of a swiss army knife that can be used to poll for safepoints, 
> handshakes, but also returns into not yet safe to
> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and 
> other thread oops) in a state of a mess in
> the GC checkpoint safepoints, rather than processing all threads and their 
> stacks. Processing is initialized
> automagically when threads wake up for a safepoint, or get poked by a 
> handshake or safepoint. Said initialization
> processes a few (3) frames and other thread oops. The rest - the bulk of the 
> frame processing, is deferred until it is
> actually needed. It is needed when a frame is exposed to either 1) execution 
> (returns or unwinding due to exception
> handling), or 2) stack walker APIs. A hook is then run to go and finish the 
> lazy processing of frames.  Mutator and GC
> threads can compete for processing. The processing is therefore performed 
> under a per-thread lock. Note that disarming
> of the poll word (that the returns are comparing against) is only performed 
> by the thread itself. So sliding the
> watermark up will require one runtime call for a thread to note that nothing 
> needs to be done, and then update the poll
> word accordingly. Downgrading the poll word concurrently by other threads was 
> simply not worth the complexity it
> brought (and is only possible on TSO machines). So left that one out.

Next set of comments.

src/hotspot/share/gc/z/zStackWatermark.cpp line 78:

> 76:   ZThreadLocalData::do_invisible_root(_jt, 
> ZBarrier::load_barrier_on_invisible_root_oop_field);
> 77:
> 78:   ZVerify::verify_thread_frames_bad(_jt);

Every time I read the name verify_thread_no_frames_bad I read it as "verify 
that no frames are bad", but that's not at
all what that function does.

Is there still a reason why verify_thread_no_frames_bad and 
verify_thread_frames_bad are split up into two functions?
Or could we fuse them into a ZVerify::verify_thread_bad?

src/hotspot/share/gc/z/zBarrier.cpp line 130:

> 128: uintptr_t 
> ZBarrier::load_barrier_on_invisible_root_oop_slow_path(uintptr_t addr) {
> 129:   return during_relocate() ? relocate(addr) : mark Publish>(addr);
> 130: }

There's a style skew between load_barrier_on_oop_slow_path and 
load_barrier_on_invisible_root_oop_slow_path. The former
calls wrapper function relocate_or_mark, which does the during_relocate() ... 
check. Maybe do the same for
load_barrier_on_invisible_root_oop_slow_path, and introduce a 
relocate_or_mark_no_follow?

src/hotspot/share/gc/z/zBarrierSet.cpp line 85:

> 83:   ZThreadLocalData::set_address_bad_mask(thread, ZAddressBadMask);
> 84:   if (thread->is_Java_thread()) {
> 85: JavaThread* const jt = static_cast(thread);

Use as_Java_thread() here?

src/hotspot/share/gc/z/zCollectedHeap.cpp line 235:

> 233:   return true;
> 234: }
> 235:

Weird placement between store barrier functions. But even weirder that we still 
have those functions. I'll remove them
with JDK-8253516.

src/hotspot/share/gc/z/zDriver.cpp line 108:

> 106: return false;
> 107:   }
> 108:

Group needs_inactive_gc_locker, skip_thread_oop_barriers, and 
allow_coalesced_vm_operations together?

Add a comment about why we chose to skip coalescing here.

src/hotspot/share/gc/z/zHeapIterator.cpp line 90:

> 88:   virtual void do_thread(Thread* thread) {
> 89: if (thread->is_Java_thread()) {
> 90:   
> StackWatermarkSet::finish_processing(static_cast(thread), NULL 
> /* context */,
> StackWatermarkKind::gc);

as_Java_thread() ?

src/hotspot/share/gc/z/zNMethod.cpp line 244:

> 242:   ZNMethodToOopsDoClosure(OopClosure* cl, bool should_disarm_nmethods) :
> 243: _cl(cl),
> 244: _should_disarm_nmethods(should_disarm_nmethods) {}

Indentation is off.

src/hotspot/share/gc/z/zObjectAllocator.cpp line 2:

> 1: /*
> 2:  * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights 
> reserved.

Revert.

src/hotspot/share/gc/z/zRootsIterator.cpp line 200:

> 198:   ZRootsIteratorClosure* const _cl;
> 199:   // The resource mark is needed because interpreter oop maps are not 
> reused in concurrent mode.
> 200:   // Instead, they are temporary, and resource allocated.

temporary, and => temporary and ?

src/hotspot/share/gc/z/zStackWatermark.cpp line 96:

> 94: void ZStackWatermark::process(const frame& fr, RegisterMap& register_map, 
> void* context) {
> 95:   ZVerify::verify_frame_bad(fr, register_map);
> 96:   frame(fr).oops_do(closure_from_context(context), &_cb_cl, 
> _map);

With recent frame::oops_do cleanups, frame(fr) can be 

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

[Stefan Karlsson]

On Tue, 22 Sep 2020 11:43:41 GMT, Erik Österlund  wrote:

> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack 
> Processing" (cf.
> https://openjdk.java.net/jeps/376).
> Basically, this patch modifies the epilog safepoint when returning from a 
> frame (supporting interpreter frames, c1, c2,
> and native wrapper frames), to compare the stack pointer against a 
> thread-local value. This turns return polls into
> more of a swiss army knife that can be used to poll for safepoints, 
> handshakes, but also returns into not yet safe to
> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and 
> other thread oops) in a state of a mess in
> the GC checkpoint safepoints, rather than processing all threads and their 
> stacks. Processing is initialized
> automagically when threads wake up for a safepoint, or get poked by a 
> handshake or safepoint. Said initialization
> processes a few (3) frames and other thread oops. The rest - the bulk of the 
> frame processing, is deferred until it is
> actually needed. It is needed when a frame is exposed to either 1) execution 
> (returns or unwinding due to exception
> handling), or 2) stack walker APIs. A hook is then run to go and finish the 
> lazy processing of frames.  Mutator and GC
> threads can compete for processing. The processing is therefore performed 
> under a per-thread lock. Note that disarming
> of the poll word (that the returns are comparing against) is only performed 
> by the thread itself. So sliding the
> watermark up will require one runtime call for a thread to note that nothing 
> needs to be done, and then update the poll
> word accordingly. Downgrading the poll word concurrently by other threads was 
> simply not worth the complexity it
> brought (and is only possible on TSO machines). So left that one out.

Partially re-reviewed the changes.

src/hotspot/cpu/x86/templateInterpreterGenerator_x86_64.cpp line 194:

> 192:
> 193: Label slow_path;
> 194: __ safepoint_poll(slow_path, r15_thread, true /* at_return */, false 
> /* in_nmethod */);

Why is this tagged with 'true /* at_return */? Same for line 240. The _x86_32 
version uses false.

src/hotspot/share/c1/c1_Runtime1.cpp line 515:

> 513:   if (thread->last_frame().is_runtime_frame()) {
> 514: // The Runtime1::handle_exception_from_callee_id handler is invoked 
> after the
> 515: // frame has been unwinded. It instead builds its own stub frame, to 
> call the

Unwided -> unwound, maybe? Same below and probably other places as well.

src/hotspot/share/compiler/oopMap.cpp line 243:

> 241:   } else {
> 242: all_do(fr, reg_map, f, process_derived_oop, _nothing_cl);
> 243:   }

I wonder if we shouldn't hide the StackWatermarkSet in the GC code, and not 
"activate" the DerivedPointerTable when a
SWS is used? Isn't it the case that already don't enable the table for ZGC? 
Couldn't this simply be: `
  if (DerivedPointerTable::is_active()) {
all_do(fr, reg_map, f, add_derived_oop, _nothing_cl);
  } else {
all_do(fr, reg_map, f, process_derived_oop, _nothing_cl);
  }
`

src/hotspot/share/compiler/oopMap.cpp line 312:

> 310: #ifdef ASSERT
> 311: if uintptr_t)loc & (sizeof(oop)-1)) != 0) ||
> 312: 
> !Universe::heap()->is_in_or_null((oop)NativeAccess::oop_load()))
>  {

Discussed offline. This is technical debt that we'd like to get rid of somehow.

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing

[Stefan Karlsson]

On Tue, 22 Sep 2020 11:43:41 GMT, Erik Österlund  wrote:

> This PR the implementation of "JEP 376: ZGC: Concurrent Thread-Stack 
> Processing" (cf.
> https://openjdk.java.net/jeps/376).
> Basically, this patch modifies the epilog safepoint when returning from a 
> frame (supporting interpreter frames, c1, c2,
> and native wrapper frames), to compare the stack pointer against a 
> thread-local value. This turns return polls into
> more of a swiss army knife that can be used to poll for safepoints, 
> handshakes, but also returns into not yet safe to
> expose frames, denoted by a "stack watermark".  ZGC will leave frames (and 
> other thread oops) in a state of a mess in
> the GC checkpoint safepoints, rather than processing all threads and their 
> stacks. Processing is initialized
> automagically when threads wake up for a safepoint, or get poked by a 
> handshake or safepoint. Said initialization
> processes a few (3) frames and other thread oops. The rest - the bulk of the 
> frame processing, is deferred until it is
> actually needed. It is needed when a frame is exposed to either 1) execution 
> (returns or unwinding due to exception
> handling), or 2) stack walker APIs. A hook is then run to go and finish the 
> lazy processing of frames.  Mutator and GC
> threads can compete for processing. The processing is therefore performed 
> under a per-thread lock. Note that disarming
> of the poll word (that the returns are comparing against) is only performed 
> by the thread itself. So sliding the
> watermark up will require one runtime call for a thread to note that nothing 
> needs to be done, and then update the poll
> word accordingly. Downgrading the poll word concurrently by other threads was 
> simply not worth the complexity it
> brought (and is only possible on TSO machines). So left that one out.

I've pre-reviewed most of the changes.

-

PR: https://git.openjdk.java.net/jdk/pull/296

Re: RFR: 8251835: 8251374 breaks jmap -dump:all

[Stefan Karlsson]

On 2020-08-14 18:39, Hohensee, Paul wrote:

Makes sense to me to do a followup. I've filed 
https://bugs.openjdk.java.net/browse/JDK-8251848.


Great.



I ran TEST="test/jdk/sun/tools/jmap/BasicJMapTest.java" 
JTREG="JAVA_OPTIONS=-XX:+UseParallelGC -XX:ParallelGCThreads=100" successfully, including 
your patch for 8251570.

This 8251835 patch looks good to me.


Thanks!

StefanK



Thanks,
Paul

On 8/14/20, 7:49 AM, "Stefan Karlsson"  wrote:

 Hi all,

 Please review this patch to fix a recently introduced jmap bug.

 https://cr.openjdk.java.net/~stefank/8251835/webrev.01/
 https://bugs.openjdk.java.net/browse/JDK-8251835

 I added the same kind of checks that we have in histo.

 Testing:
 - Tested locally with the failing test
 - Tier1-tier5 on Linux x64

 Paul posted a slightly more elaborate fix that makes dump more akin to
 histo:
 http://cr.openjdk.java.net/~phh/8251835/webrev.00/

 I don't know the testing status of that patch. If this needs to be fixed
 ASAP, I propose my fix, and then add the rest of Pauls bits as a
 follow-up RFE. If we have time to run Paul's patch through testing, then
 I'm fine with that as well.

 Thanks,
 StefanK

RFR: 8251835: 8251374 breaks jmap -dump:all

[Stefan Karlsson]

Hi all,

Please review this patch to fix a recently introduced jmap bug.

https://cr.openjdk.java.net/~stefank/8251835/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8251835

I added the same kind of checks that we have in histo.

Testing:
- Tested locally with the failing test
- Tier1-tier5 on Linux x64

Paul posted a slightly more elaborate fix that makes dump more akin to 
histo:

http://cr.openjdk.java.net/~phh/8251835/webrev.00/

I don't know the testing status of that patch. If this needs to be fixed 
ASAP, I propose my fix, and then add the rest of Pauls bits as a 
follow-up RFE. If we have time to run Paul's patch through testing, then 
I'm fine with that as well.


Thanks,
StefanK

Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

[Stefan Karlsson]

On 2020-08-05 07:22, linzang(臧琳) wrote:

Hi Serguei,

No problem, Thanks for your reviewing :)

    I wll upload a new webrev later, so may I ask your help to review it 
again?


Hi Stefan,

    As Paul mentioned, the _/missed/_count is not a size,  so size_t may 
not be clear, what’s your opinion about uint64_t?


We typically don't restrict the usage of size_t to only *sizes* in the 
HotSpot. If you search the code you'll find many count variables using 
size_t, so I personally don't see the need to change the type.


However, if you really do want to change it then maybe using another 
type that is 32 bits on 32-bit machines, maybe uintx? IIRC, using 
uint64_t and some of the Atomics operations are problematic on some 
32-bit platforms, so using a type that matches the word size of the 
targetted machine helps you not having to think about that.




    It seems the uint overflow may happened on 64bit machine with large 
heap, e.g. may be more than 4 Giga objects (8byte header + 8 byte 
klassptr + 8byte field) in a heap that is larger than 96 GB,  uint64_t 
is ok in this case.


Exactly.

Thanks,
StefanK



BRs,

Lin

*From: *"serguei.spit...@oracle.com" 
*Date: *Wednesday, August 5, 2020 at 1:02 PM
*To: *"linzang(臧琳)" , "Hohensee, Paul" 
, Stefan Karlsson , 
David Holmes , serviceability-dev 
, "hotspot-gc-...@openjdk.java.net" 

*Subject: *Re: RFR(L): 8215624: add parallel heap inspection support for 
jmap histo(G1)(Internet mail)


Oh, sorry for the confusion, please, skip my question. :)
C++ does not have the '&&=' operator.

Thanks,
Serguei

On 8/4/20 21:56, serguei.spit...@oracle.com 
<mailto:serguei.spit...@oracle.com> wrote:


Hi Lin,


https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/src/hotspot/share/memory/heapInspection.cpp.udiff.html

+class KlassInfoTableMergeClosure : public KlassInfoClosure {

+private:

+  KlassInfoTable* _dest;

+  bool _success;

+public:

+  KlassInfoTableMergeClosure(KlassInfoTable* table) : _dest(table),
_success(true) {}

+  void do_cinfo(KlassInfoEntry* cie) {

+    _success &= _dest->merge_entry(cie);

+  }

The operator '&=' above looks strange.
Did you actually want to use the operator '&&=' instead? :

+    _success &&= _dest->merge_entry(cie);


Thanks,
Serguei




On 8/3/20 07:51, linzang(臧琳) wrote:

Dear Stefan,

  May I ask your help to review again? I have made a delta 
based on the last changeset you have reviewed(webrev04),hope it could ease your 
reviewing work.

  
webrev:https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/

  delta (vs 
webrev04):https://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/delta_10vs04/webrev/

  bug:https://bugs.openjdk.java.net/browse/JDK-8215624

  CSR(approved):https://bugs.openjdk.java.net/browse/JDK-8239290

  


BRs,

Lin

On 2020/7/30, 5:21 AM, "Hohensee, Paul"  
<mailto:hohen...@amazon.com>  wrote:

     A submit repo run with this succeeded, so afaic you're good to go. 
Stefan, you reviewed the GC part before, it'd be great if you could ok the 
final version.

     Thanks,

     Paul

     On 7/29/20, 5:02 AM, "linzang(臧琳)"  
<mailto:linz...@tencent.com>  wrote:

     Upload a new change 
athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_10/

     It fix an issue of windows fail :

     

     In heapInspect.cpp

     - size_t HeapInspection::populate_table(KlassInfoTable* cit, 
BoolObjectClosure *filter, uint parallel_thread_num) {

     + uint HeapInspection::populate_table(KlassInfoTable* cit, 
BoolObjectClosure *filter, uint parallel_thread_num) {

     

     In heapInspect.hpp

     - size_t populate_table(KlassInfoTable* cit, 
BoolObjectClosure* filter = NULL, uint parallel_thread_num = 1) 
NOT_SERVICES_RETURN_(0);

     +  uint populate_table(KlassInfoTable* cit, BoolObjectClosure* 
filter = NULL, uint parallel_thread_num = 1) NOT_SERVICES_RETURN_(0);

     

     BRs,

     Lin

     On 2020/7/27, 11:26 AM, "linzang(臧琳)"  
<mailto:linz...@tencent.com>  wrote:

     I update a new change 
athttp://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_09

     It includes a tiny fix of build failure on windows:

     

     In attachListener.cpp:

     -  uint parallel_thread_num = MAX(1, 
(uint)os::initial_active_process

Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

[Stefan Karlsson]

inal return in ParHeapInspectTask::work, 
you mentioned it but seems not include in the webrev :-)
 • delete a unnecessary blank line in heapInspect.cpp 
at merge_entry()

 
#
 --- old/src/hotspot/share/memory/heapInspection.cpp 
2020-07-23 11:23:29.281666456 +0800
 +++ new/src/hotspot/share/memory/heapInspection.cpp 
2020-07-23 11:23:29.017666447 +0800
 @@ -251,7 +251,6 @@
  _size_of_instances_in_words += cie->words();
  return true;
}
 -
return false;
  }

 @@ -568,7 +567,6 @@
  Atomic::add(&_missed_count, missed_count);
} else {
  Atomic::store(&_success, false);
 -   return;
}
  }
 
#


 Here is the webrev  
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_08/

 BRs,
 Lin
 -
 From: "Hohensee, Paul" 
 Date: Thursday, July 23, 2020 at 6:48 AM
 To: "linzang(臧琳)" , Stefan Karlsson , 
"serguei.spit...@oracle.com" , David Holmes , serviceability-dev 
, "hotspot-gc-...@openjdk.java.net" 
 Subject: RE: RFR(L): 8215624: add parallel heap inspection 
support for jmap histo(G1)(Internet mail)

 Just small things.

 heapInspection.cpp:

 In ParHeapInspectTask::work, remove the final return statement 
and fix the following ‘}’ indent. I.e., replace

 +Atomic::store(&_success, false);
 +return;
 +   }

 with

 +Atomic::store(&_success, false);
 +  }

 In HeapInspection::heap_inspection, missed_count should be a 
uint to match other missed_count declarations, and should be initialized to the 
result of populate_table() rather than separately to 0.

 attachListener.cpp:

 In heap_inspection, initial_processor_count returns an int, so 
cast its result to a uint.

 Similarly, parse_uintx returns a uintx, so cast its result 
(num) to uint when assigning to parallel_thread_num.

 BasicJMapTest.java:

 I took the liberty of refactoring 
testHisto*/histoToFile/testDump*/dump to remove redundant interposition methods 
and make histoToFile and dump look as similar as possible.

 Webrev with the above changes in

 http://cr.openjdk.java.net/~phh/8214535/webrev.01/

 Thanks,
 Paul

 On 7/15/20, 2:13 AM, "linzang(臧琳)"  wrote:

  Upload a new webrev at 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07/
  It fix a potential issue that unexpected number of threads maybe 
calculated for "parallel" option of jmap -histo in container.
 As shown at 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_07-delta/src/hotspot/share/services/attachListener.cpp.udiff.html

 ### attachListener.cpp 
 @@ -252,11 +252,11 @@
  static jint heap_inspection(AttachOperation* op, 
outputStream* out) {
bool live_objects_only = true;   // default is true to 
retain the behavior before this change is made
outputStream* os = out;   // if path not specified or 
path is NULL, use out
fileStream* fs = NULL;
const char* arg0 = op->arg(0);
 -  uint parallel_thread_num = MAX(1, os::processor_count() 
* 3 / 8); // default is less than half of processors.
 +  uint parallel_thread_num = MAX(1, 
os::initial_active_processor_count() * 3 / 8); // default is less than half of 
processors.
if (arg0 != NULL && (strlen(arg0) > 0)) {
  if (strcmp(arg0, "-all") != 0 && strcmp(arg0, 
"-live") != 0) {
out->print_cr("Invalid argument to inspectheap 
operation: %s", arg0);
return JNI_ERR;
  }
 ###

 Thanks.

 BRs,
Lin

 On 2020/7/9, 3:22 PM, "linzang(臧琳)"  
wrote:

   

Re: RFR 8247808: Move JVMTI strong oops to OopStorage

[Stefan Karlsson]

Hi Coleen,

On 2020-06-17 23:25, coleen.phillim...@oracle.com wrote:

Summary: Remove JVMTI oops_do calls from JVMTI and GCs

Tested with tier1-3, also built shenandoah to verify shenandoah changes.

open webrev at http://cr.openjdk.java.net/~coleenp/2020/8247808.01/webrev


https://cr.openjdk.java.net/~coleenp/2020/8247808.01/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.udiff.html

 JvmtiBreakpoint::~JvmtiBreakpoint() {
- if (_class_holder != NULL) {
- NativeAccess<>::oop_store(_class_holder, (oop)NULL);
- OopStorageSet::vm_global()->release(_class_holder);
+ if (_class_holder.resolve() != NULL) {
+ _class_holder.release();
   }
 }

Could this be changed to peek() / release() instead? The resolve() call 
is going to keep the object alive until next for ZGC marking cycle.


The rest looks OK.

Below are some comments about things that I find odd and non-obvious 
from reading the code, and may be potentials for cleanups to make it 
easier for the next to understand the code:


The above code assumes that as soon as OopHandle::create has been 
called, we won't store NULL into the _obj pointer. If someone does, then 
we would leak the memory. OopHandle has a function ptr_raw, that allows 
someone to clear the _obj pointer. I have to assume that this function 
isn't used in this code.


---

 214 void JvmtiBreakpoint::copy(JvmtiBreakpoint& bp) {
 215   _method   = bp._method;
 216   _bci  = bp._bci;
217 _class_holder = OopHandle::create(bp._class_holder.resolve());
 218 }

This one looks odd, because the _class_holder is overwritten without 
releasing the old OopHandle. This is currently OK, because copy is only 
called from clone, which just created a new JvmtiBreakpoint:


  GrowableElement *clone()    {
    JvmtiBreakpoint *bp = new JvmtiBreakpoint();
    bp->copy(*this);
    return bp;
  }

 I think this would have been much more obvious if copy/clone were a 
copy constructor.


With that said, it looks like we now have two JvmtiBreakpoints with the 
same OopHandle contents. So, OopHandle::release will be called twice. 
Now that works because release clears the oop value:


inline void OopHandle::release() {
  // Clear the OopHandle first
  NativeAccess<>::oop_store(_obj, (oop)NULL);
  OopStorageSet::vm_global()->release(_obj);
}

and the resolve() != NULL check will prevent the OopHandle from being 
released twice:


+ if (_class_holder.resolve() != NULL) {
+ _class_holder.release();
   }

StefanK


bug link https://bugs.openjdk.java.net/browse/JDK-8247808

Thanks,
Coleen

Re: RFR(S): JDK-8247362 HeapDumpCompressedTest.java#id0 fails due to "Multiple garbage collectors selected"

[Stefan Karlsson]

Looks good.

StefanK

On 2020-06-10 23:00, Schmelter, Ralf wrote:


Hi,

https://bugs.openjdk.java.net/browse/JDK-8237354 added a test, which 
did not properly protect against explicitly set GCs (for serial, 
parallel and G1 GC). This fixes it by adding the corresponding 
@requires tag for each of the three GCs.


bugreport: https://bugs.openjdk.java.net/browse/JDK-8247362 



webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8247362/webrev.0/

Best regards,

Ralf

Re: RFR(T): 8244622: Remove SA's memory/FreeChunk.java. It's no longer used.

[Stefan Karlsson]

Looks good.

StefanK

On 2020-05-27 01:07, Chris Plummer wrote:

Hello,

Please review the following trivial change to remove FreeChunk.java:

https://bugs.openjdk.java.net/browse/JDK-8244622
http://cr.openjdk.java.net/~cjplummer/8244622/webrev.00/index.html

Tested with tier1 and also running all SA tests.

thanks,

Chris

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (hotspot)

[Stefan Karlsson]

Hi Mikael,

On 2020-05-04 07:12, Mikael Vidstedt wrote:


Please review this change which implements part of JEP 381:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/hotspot/open/webrev/


I went over this patch and collected some comments:

src/hotspot/share/adlc/output_c.cpp
src/hotspot/share/adlc/output_h.cpp

Awkward code layout after change to.


src/hotspot/share/c1/c1_Runtime1.cpp
src/hotspot/share/classfile/classListParser.cpp
src/hotspot/share/memory/arena.hpp
src/hotspot/share/opto/chaitin.cpp
test/hotspot/jtreg/gc/TestCardTablePageCommits.java

Surrounding comments still refers to Sparc and/or Solaris.

There are even more places if you search in the rest of the HotSpot 
source. Are we leaving those for a separate cleanup pass?



src/hotspot/share/gc/g1/g1HeapRegionAttr.hpp

Remove comment:
  // We use different types to represent the state value depending on 
platform as

  // some have issues loading parts of words.


src/hotspot/share/gc/shared/memset_with_concurrent_readers.hpp

Fuse the declaration and definition, now that we only have one 
implementation. Maybe even remove function/file at some point.



src/hotspot/share/utilities/globalDefinitions.hpp

Now that STACK_BIAS is always 0, should we remove its usages? Follow-up RFE?


src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java

Maybe remove decryptSuffix?


src/utils/hsdis/Makefile

Is this really correct?

Shouldn't:
ARCH1=$(CPU:x86_64=amd64)
ARCH2=$(ARCH1:i686=i386)
ARCH=$(ARCH2:sparc64=sparcv9)

be changed to:
ARCH1=$(CPU:x86_64=amd64)
ARCH=$(ARCH1:i686=i386)

so that we have ARCH defined?


Other than that this looks good to me.

StefanK


JEP: https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove the Solaris and SPARC ports. The other patches are 
being sent out for review to mailing lists appropriate for the respective areas 
the touch. An email will be sent to jdk-dev summarizing all the 
patches/reviews. To be clear: this patch is *not* in itself complete and 
stand-alone - all of the (six) patches are needed to form a complete patch. 
Some changes in this patch may look wrong or incomplete unless also looking at 
the corresponding changes in other areas.

For convenience, I’m including a link below[1] to the full webrev, but in case 
you have comments on changes in other areas, outside of the files included in 
this thread, please provide those comments directly in the thread on the 
appropriate mailing list for that area if possible.

In case it helps, the changes were effectively produced by searching for and 
updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
More information about the areas impacted can be found in the JEP itself.

A big thank you to Igor Ignatyev for helping make the changes to the hotspot 
tests!

Also, I have a short list of follow-ups which I’m going to look at separately 
from this JEP/patch, mainly related to command line options/flags which are no 
longer relevant and should be deprecated/obsoleted/removed.

Testing:

A slightly earlier version of this change successfully passed tier1-8, as well 
as client tier1-2. Additional testing will be done after the first round of 
reviews has been completed.

Cheers,
Mikael

[1] 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

Re: RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

On 2020-05-01 21:34, Chris Plummer wrote:

On 4/30/20 2:07 AM, Stefan Karlsson wrote:


...


There was one odd thing in jdi that requires extra scrutiny:
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html 

Yes, that did look a odd at first glance, but I think that fact that 
your removed addTestVmAndJavaOptions() and everything still built is 
proof enough that it was just bit rotted code and not needed.


Thanks for checking on this.

StefanK



Chris


I've run this through tier1-3, and are currently running this through 
higher tiers.


Thanks,
StefanK

Re: RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

On 2020-04-30 12:22, Stefan Karlsson wrote:

Hi David,

On 2020-04-30 11:59, David Holmes wrote:

...


---

test/hotspot/jtreg/gc/arguments/GCArguments.java

Isn't the String[] <-> List conversion already handled in 
ProcessTools?


This looks like an area where GC added its own helper utilities early 
on and they aren't really needed any more. Future RFE?
I thought the same when I first looked at this, but there's a subtle 
withDefaults(...) call in there that filters out some of the passed in 
flags.


However, it's weird because it doesn't filter the test.vm.opts and 
test.java.opts, only the flags that the tests explicitly passes in. I 
think we need to take a closer look at this, as a separate RFE.


I read the disableX part to quickly. This *adds* flags to turn off features.

StefanK

Re: RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

Hi David,

On 2020-04-30 11:59, David Holmes wrote:

Hi Stefan,

On 30/04/2020 7:07 pm, Stefan Karlsson wrote:

Hi all,

Please review this patch to make it less likely that we accidentally 
add or fail to add test.java.opts and test.vm.opts to our spawned 
test JVMs.


https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8244078

ProcessTools.createJavaProcessBuilder(cmd) creates a ProcessBuilder 
*without* test.java.opts and test.vm.opts. There is a 
(addTestVmAndJavaOptions, cmd) overload that allows the caller to 
opt-in to the addition of these flags. The created ProcessBuilder is 
then used to start the JVM, and almost always fed into an 
OutputAnalyzer.


There's another function executeTestJvm, that both creates a 
ProcessBuilder and then feeds it into an OutputAnalyzer (plus a bit 
more). This function uses createJavaProcessBuilder(true, cmd), and 
thereby adds the test.java.opts and test.vm.opts flags.


This means that one has to know about this difference when reading 
code using createJavaProcessBuilder(cmd) and executeTestJvm(cmd), and 
when creating (copying) code using these functions.


It has been suggested that createJavaProcessBuilder is intended to be 
a lower-level, building block API and that it's not confusing that 
they have different behavior. I don't really agree, but I'm buying 
into the notion of lower-level vs higher-level APIs here. So, my 
proposal is to remove the addTestVmAndJavaOptions feature from 
createJavaProcessBuilder, and instead create a new function called 
createTestJvm that adds test.java.opts and test.vm.opts. The name is 
intentionally similar to executeTestJvm, and in fact, the 
executeTestJvm implementation will use createTestJvm:


  public static OutputAnalyzer executeTestJvm(String... cmds) 
throws Exception {

- ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
+ ProcessBuilder pb = createTestJvm(cmds);
  return executeProcess(pb);
  }

The rest of the patch is mainly:
- leaving createJavaProcessBuilder(cmd) as is
- replacing createJavaProcessBuilder(false, cmd) with 
createJavaProcessBuilder(cmd)

- replacing createJavaProcessBuilder(true, cmd) with createTestJvm(cmd)


This all looks good to me. I think this high/low API makes things much 
clearer.


An observation from compiler/runtime/cr8015436/Driver8015436.java - can:

oa = ProcessTools.executeProcess(ProcessTools.createTestJvm(...));

be replaced with

oa = executeTestJvm(...);

?


Yes. Good point.



---

test/hotspot/jtreg/gc/arguments/GCArguments.java

Isn't the String[] <-> List conversion already handled in 
ProcessTools?


This looks like an area where GC added its own helper utilities early 
on and they aren't really needed any more. Future RFE?
I thought the same when I first looked at this, but there's a subtle 
withDefaults(...) call in there that filters out some of the passed in 
flags.


However, it's weird because it doesn't filter the test.vm.opts and 
test.java.opts, only the flags that the tests explicitly passes in. I 
think we need to take a closer look at this, as a separate RFE.




---

test/hotspot/jtreg/gc/g1/mixedgc/TestLogging.java
test/hotspot/jtreg/gc/nvdimm/*

Just observation - these tests also use a code pattern that looks like 
it could use executeTestJvm instead.

Are you thinking about:
    ProcessBuilder pb = ProcessTools.createTestJvm(flags);
    OutputAnalyzer output = new OutputAnalyzer(pb.start());

Yes, my goal is to be able to replace those line with a one-liner. There 
is huge number of places were we have this pattern.


However, note that executeTestJvm waits for the process to finish and 
dumps the output to files. new OutputAnalyzer(pb.start()) doesn't 
AFAICT. Either we think executeTestJvm is the right approach, or we have 
to create yet another function that performs the above code.




---

test/lib/jdk/test/lib/process/ProcessTools.java

This looks a bit odd:

   /**
 * @see #executeTestJvm(String...)
 * @param cmds User specified arguments.
 * @return The output from the process.
 */
    public static OutputAnalyzer executeTestJava(String... cmds) 
throws Exception {

    return executeTestJvm(cmds);
    }

I assume this exists because this was the name used in the JDK test 
library originally and many (most?) of the JDK tests call it rather 
than executeTestJvm. There are a couple of hotspot callers that should 
probably be converted over:


./jtreg/compiler/jvmci/errors/TestInvalidTieredStopAtLevel.java
./jtreg/compiler/jvmci/TestValidateModules.java

otherwise another future RFE to switch over to single API.


I think this is what Alan mentioned in his mail. I thought the 
executeTestJava call was the deprecated one, but apparently it's the 
other way around.





There was one odd thing in jdi that requires extra scrutiny:
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udif

Re: RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

On 2020-04-30 11:24, Alan Bateman wrote:

On 30/04/2020 10:07, Stefan Karlsson wrote:

Hi all,

Please review this patch to make it less likely that we accidentally 
add or fail to add test.java.opts and test.vm.opts to our spawned 
test JVMs.


https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8244078
We tried a few years ago to get the tests in the libraries areas moved 
to using the xxxJava variants of these methods to make it clear that 
it exec'ing the java launcher of the JDK under test. Methods such as 
executeTestJvm were deprecated as the naming didn't make it clear that 
it exec's the java launcher. Looks like some of this has been lost 
with the combining of the test infrastructure. So while not directly 
to this webrev, I think we need to go back to the naming issue at some 
point and avoid having two sets of methods that do the same thing.


Are you specifically referring to executeTestJvm vs executeTestJava?

We could take a step back and look at all these functions and try to 
find good names and/or default values. If we're going to clean this up, 
I think we need to figure out good and, preferably, concise naming for 
all of these:

- create test java launcher with test.*.opts
- create test java launcher without test.*.opts
- execute test java launcher with test.*opts
- execute test java launcher without test.*.opts

Do you want me to hold off on this patch until we've resolved this?

StefanK



-Alan

RFR: 8244078: ProcessTools executeTestJvm and createJavaProcessBuilder have inconsistent handling of test.*.opts

[Stefan Karlsson]

Hi all,

Please review this patch to make it less likely that we accidentally add 
or fail to add test.java.opts and test.vm.opts to our spawned test JVMs.


https://cr.openjdk.java.net/~stefank/8244078/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8244078

ProcessTools.createJavaProcessBuilder(cmd) creates a ProcessBuilder 
*without* test.java.opts and test.vm.opts. There is a 
(addTestVmAndJavaOptions, cmd) overload that allows the caller to opt-in 
to the addition of these flags. The created ProcessBuilder is then used 
to start the JVM, and almost always fed into an OutputAnalyzer.


There's another function executeTestJvm, that both creates a 
ProcessBuilder and then feeds it into an OutputAnalyzer (plus a bit 
more). This function uses createJavaProcessBuilder(true, cmd), and 
thereby adds the test.java.opts and test.vm.opts flags.


This means that one has to know about this difference when reading code 
using createJavaProcessBuilder(cmd) and executeTestJvm(cmd), and when 
creating (copying) code using these functions.


It has been suggested that createJavaProcessBuilder is intended to be a 
lower-level, building block API and that it's not confusing that they 
have different behavior. I don't really agree, but I'm buying into the 
notion of lower-level vs higher-level APIs here. So, my proposal is to 
remove the addTestVmAndJavaOptions feature from 
createJavaProcessBuilder, and instead create a new function called 
createTestJvm that adds test.java.opts and test.vm.opts. The name is 
intentionally similar to executeTestJvm, and in fact, the executeTestJvm 
implementation will use createTestJvm:


 public static OutputAnalyzer executeTestJvm(String... cmds) throws 
Exception {
- ProcessBuilder pb = createJavaProcessBuilder(true, cmds);
+ ProcessBuilder pb = createTestJvm(cmds);
 return executeProcess(pb);
 }

The rest of the patch is mainly:
- leaving createJavaProcessBuilder(cmd) as is
- replacing createJavaProcessBuilder(false, cmd) with 
createJavaProcessBuilder(cmd)

- replacing createJavaProcessBuilder(true, cmd) with createTestJvm(cmd)

There was one odd thing in jdi that requires extra scrutiny:
https://cr.openjdk.java.net/~stefank/8244078/webrev.01/test/jdk/com/sun/jdi/lib/jdb/Debuggee.java.udiff.html

I've run this through tier1-3, and are currently running this through 
higher tiers.


Thanks,
StefanK

Re: RFR(S/T) : 8243568 : serviceability/logging/TestLogRotation.java uses 'test.java.opts' and not 'test.vm.opts'

[Stefan Karlsson]

Looks good.

StefanK

On 2020-04-25 00:30, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8243568/webrev.00

8 lines changed: 0 ins; 6 del; 2 mod;

Hi all,

could you please review this small and trivial patch which updates 
serviceability/logging/TestLogRotation.java test to pass both 'test.java.opts' 
and not 'test.vm.opts' ? to do that, the patch removes the custom logic of 
handling test.java.opts and just uses 
ProcessTools.createJavaProcessBuilder(boolean addTestVmAndJavaOptions=true, 
String...), which prepends values of both properties.
from JBS:

serviceability/logging/TestLogRotation.java test use 'test.java.opts' to get 
external vm flags, yet actually external flags are split b/w 'test.java.opts' 
and 'test.vm.opts' and in the majority of cases all external flags are set 
listed in 'test.vm.opts' so the test should be updated to read both.

JBS: https://bugs.openjdk.java.net/browse/JDK-8243568
webrev: http://cr.openjdk.java.net/~iignatyev//8243568/webrev.00

Thanks,
-- Igor

Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)(Internet mail)

[Stefan Karlsson]

Hi Lin,

On 2020-04-26 05:10, linzang(臧琳) wrote:

Hi Stefan and Paul，
 I have made a new patch based on your comments and Stefan's Poc code:
 Webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03/
 Delta(based on Stefan's change:) : 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_03-delta/webrev_03-delta/


Thanks for providing a delta patch. It makes it much easier to look at, 
and more likely for reviewers to continue reviewing.


I'm going to continue focusing on the GC parts, and leave the rest to 
others to review.




 And Here are main changed I made and want to discuss with you:
 1.  changed"parallelThreadNum=" to "parallel=" for jmap -histo options.
 2.  Add logic to test where parallelHeapInspection is fail, in 
heapInspection.cpp
   This is because the parHeapInspectTask create thread local 
KlassInfoTable in it's work() method, and this may fail because of native OOM, 
in this case, the parallel should fail and serial heap inspection can be tried.
   One more thing I want discuss with you is about the member "_success" of 
parHeapInspectTask, when native OOM happenes, it is set to false. And since this "set" operation 
can be conducted in multiple threads, should it be atomic ops?  IMO, this is not necessary because 
"_success" can only be set to false, and there is no way to change it from back to true after the 
ParHeapInspectTask instance is created, so it is save to be non-atomic, do you agree with that?


In these situations you should be using the Atomic::load/store 
primitives. We're moving toward a later C++ standard were data races are 
considered undefined behavior.



3. make CollectedHeap::run_task() be an abstract virtual func, so that every subclass 
of collectedHeap should support it, so later implementation of new collectedHeap will not 
miss the "parallel" features.
  The problem I want to discuss with you is about epsilonHeap and 
SerialHeap, as they may not need parallel heap iteration, so I only make 
task->work(0), in case the run_task() is invoked someway in future. Another way 
is to left run_task()  unimplemented, which one do you think is better?


I don't have a strong opinion about this.

 And also please help take a look at the zHeap, as there is a class 
zTask that wrap the abstractGangTask, and the collectedHeap::run_task() 
only accept  AbstraceGangTask* as argument, so I made a delegate class 
to adapt it , please see src/hotspot/share/gc/z/zHeap.cpp.


   There maybe other better ways to sovle the above problems, welcome for 
any comments, Thanks!


I've created a few cleanups and changes on top of your latest patch:

https://cr.openjdk.java.net/~stefank/8215624/webrev.02.delta
https://cr.openjdk.java.net/~stefank/8215624/webrev.02

- Adding Atomic::load/store.
- Removing the time measurement in the run_task. I renamed G1's function 
to run_task_timed. If we need this outside of G1, we can rethink the API 
at that point.

- ZGC style cleanups

Thanks,
StefanK



BRs,
Lin

On 2020/4/23, 11:08 AM, "linzang(臧琳)"  wrote:

 Thanks Paul! I agree with using "parallel", will make the update in next 
patch, Thanks for help update the CSR.

 BRs,
 Lin

 On 2020/4/23, 4:42 AM, "Hohensee, Paul"  wrote:

 For the interface, I'd use "parallel" instead of "parallelThreadNum". All the 
other options are lower case, and it's a lot easier to type "parallel". I took the liberty of 
updating the CSR. If you're ok with it, you might want to change variable names and such, plus of course 
JMap.usage.

 Thanks,
 Paul

 On 4/22/20, 2:29 AM, "serviceability-dev on behalf of linzang(臧琳)" 
 wrote:

 Dear Stefan,

 Thanks a lot! I agree with you to decouple the heap 
inspection code with GC's.
 I will start  from your POC code, may discuss with you 
later.


 BRs,
 Lin

 On 2020/4/22, 5:14 PM, "Stefan Karlsson" 
 wrote:

 Hi Lin,

 I took a look at this earlier and saw that the heap inspection 
code is
 strongly coupled with the CollectedHeap and G1CollectedHeap. 
I'd prefer
 if we'd abstract this away, so that the GCs only provide a 
"parallel
 object iteration" interface, and the heap inspection code is 
kept elsewhere.

 I started experimenting with doing that, but other 
higher-priority (to
 me) tasks have had to take precedence.

 I've uploaded my work-in-progress / proof-of-concept:
   https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/
   https://cr.openjdk.java.net/~stefank/8215624/webrev.01/

 

Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)

[Stefan Karlsson]

Hi Lin,

I took a look at this earlier and saw that the heap inspection code is 
strongly coupled with the CollectedHeap and G1CollectedHeap. I'd prefer 
if we'd abstract this away, so that the GCs only provide a "parallel 
object iteration" interface, and the heap inspection code is kept elsewhere.


I started experimenting with doing that, but other higher-priority (to 
me) tasks have had to take precedence.


I've uploaded my work-in-progress / proof-of-concept:
 https://cr.openjdk.java.net/~stefank/8215624/webrev.01.delta/
 https://cr.openjdk.java.net/~stefank/8215624/webrev.01/

The current code doesn't handle the lifecycle (deletion) of the 
ParallelObjectIterators. There's also code left unimplemented in around 
CollectedHeap::run_task. However, I think this could work as a basis to 
pull out the heap inspection code out of the GCs.


Thanks,
StefanK

On 2020-04-22 02:21, linzang(臧琳) wrote:

Dear all,
  May I ask you help to review? This RFR has been there for quite a while.
  Thanks!
  
BRs,

Lin

> On 2020/3/16, 5:18 PM, "linzang(臧琳)"  wrote:>


   Just update a new path, my preliminary measure show about 3.5x speedup of 
jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread 
number set to 4).
webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/
bug: https://bugs.openjdk.java.net/browse/JDK-8215624
CSR: https://bugs.openjdk.java.net/browse/JDK-8239290
BRs,
  Lin
  > On 2020/3/2, 9:56 PM, "linzang(臧琳)"  wrote:
  >
  >Dear all,
  >  Let me try to ease the reviewing work by some explanation :P
  >  The patch's target is to speed up jmap -histo for heap 
iteration, from my experience it is necessary for large heap investigation. E.g in 
bigData scenario I have tried to conduct jmap -histo against 180GB heap, it does 
take quite a while.
  >  And if my understanding is corrent, even the jmap -histo without 
"live" option does heap inspection with heap lock acquired. so it is very likely 
to block mutator thread in allocation-sensitive scenario. I would say the faster the heap 
inspection does, the shorter the mutator be blocked. This is parallel iteration for jmap is 
necessary.
  >  I think the parallel heap inspection should be applied to all 
kind of heap. However, consider the heap layout are different for  GCs, much time 
is required to understand all kinds of the heap layout to make the whole change. 
IMO, It is not wise to have a huge patch for the whole solution at once, and it is 
even harder to review it. So I plan to implement it incrementally, the first patch 
(this one) is going to confirm the implemention detail of how jmap accept the new 
option, passes it to attachListener of the jvm process and then how to make the 
parallel inspection closure be generic enough to make it easy to extend to 
different heap layout. And also how to implement the heap inspection in specific 
gc's heap. This patch use G1's heap as the begining.
  >  This patch actually do several things:
  >  1. Add an option "parallelThreadNum=" to jmap -histo, the 
default behavior is to set N to 0, means let's JVM decide how many threads to use for heap 
inspection. Set this option to 1 will disable parallel heap inspection. (more details in CSR: 
https://bugs.openjdk.java.net/browse/JDK-8239290)
  >  2. Make a change in how Jmap passing arguments, changes in 
http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html,
 originally it pass options as separate arguments to attachListener, this patch 
change to that all options be compose to a single string. So the arg_count_max in 
attachListener.hpp do not need to be changed, and hence avoid the compatibility 
issue, as disscussed at 
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html
  > 3. Add an abstract class ParHeapInspectTask in 
heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method prepares 
the data structure (KlassInfoTable) need for every parallel worker thread, and 
then call do_object_iterate_parallel() which is heap specific implementation. I 
also added some machenism in KlassInfoTable to support parallel iteration, such as 
merge().
  >4. In specific heap (G1 in this patch), create a subclass of 
ParHeapInspectTask, implement the do_object_iterate_parallel() for parallel heap 
inspection. For G1, it simply invoke g1CollectedHeap's object_iterate_parallel().
  >5. Add related test.
  >6. it may be easy to extend this patch for other kinds of heap 
by creating subclass of ParHeapInspectTask and implement the 
do_object_iterate_parallel().
  >
  >Hope these info could help on code review and initate the discussion 
:-)
  >Thanks!
  >
  >BRs,
  >Lin
  >>On 2020/2/19, 9:40 AM, 

Re: RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified

[Stefan Karlsson]

Thanks for changing the name. Sounds good to me. I leave the full review 
to others.


StefanK

On 2020-03-25 20:01, Leonid Mesnik wrote:


Added Ioi, who also proposed new version of startAppVmOpts.

Please find new webrev: 
http://cr.openjdk.java.net/~lmesnik/8240698/webrev.01/


Renamed startAppVmOpts/runAppVmOpts to 
"startAppExactJvmOpts/runAppExactJvmOpts" is used. It should make very 
clear that this method doesn't use any of test.java.opts, test.vm.opts.


Also, I fixed test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java 
metnioned by Igor, and removed null pointer check as Ioi suggested in 
startApp method.


+ public static void startApp(LingeredApp theApp, String... 
additionalJvmOpts) throws IOException {
+ startAppExactJvmOpts(theApp, 
Utils.appendTestJavaOpts(additionalJvmOpts));

+ }

Leonid

On 3/25/20 10:14 AM, Stefan Karlsson wrote:

On 2020-03-25 17:40, Igor Ignatyev wrote:

Hi Leonid,

I have briefly looked at the patch, a few comments so far:

test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java:
  - at L#114, could you please call static method using class name 
(as the opposite of using instance)? or was it meant to be 
theApp.runAppVmOpts(vmArgs) ?


test/lib/jdk/test/lib/apps/LingeredApp.java:
- it seems that code indent of startApp(LingeredApp, String[]) isn't 
correct
- I don't like startAppVmOpts name, but unfortunately don't have a 
better suggestion (yet)


I was going to say the same. Jtreg has the concept of "java options" 
and "vm options". We have had a fair share of bugs and wasted time 
when tests have been using the "vm options" part (VM_OPTIONS, 
test.vm.options, etc), and we've been moving away from using that way 
to pass options. I recently cleaned up some of this with:


8237111: LingeredApp should be started with getTestJavaOpts

Because of this, I would prefer if we used a name that doesn't 
include "VmOpts", because it's too alike the other concept. Some 
suggestions:

 startAppJavaOptions
 startAppUsingJavaOptions
 startAppWithJavaOptions
 startAppExactJavaOptions
 startAppJvmOptions

Thanks,
StefanK


Thanks,
-- Igor

On Mar 25, 2020, at 8:55 AM, Leonid Mesnik 
 wrote:


Hi

Could you please review following fix which change LingeredApp to 
prepend vm options to java/vm.test.opts when startApp is used and 
provide startAppVmOpts to override options completely.


The intention is to avoid issue like in this bug where test/jtreg 
options were ignored by tests. Also I fixed some tests where 
intention was to append vm options rather than to override them.


webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/

bug: https://bugs.openjdk.java.net/browse/JDK-8240698

Leonid

Re: RFR: 8240698: LingeredApp does not pass getTestJavaOpts() to the children process if vmArguments is already specified

[Stefan Karlsson]

On 2020-03-25 17:40, Igor Ignatyev wrote:

Hi Leonid,

I have briefly looked at the patch, a few comments so far:

test/hotspot/jtreg/serviceability/sa/ClhsdbFlags.java:
  - at L#114, could you please call static method using class name (as the 
opposite of using instance)? or was it meant to be theApp.runAppVmOpts(vmArgs) ?

test/lib/jdk/test/lib/apps/LingeredApp.java:
- it seems that code indent of startApp(LingeredApp, String[]) isn't correct
- I don't like startAppVmOpts name, but unfortunately don't have a better 
suggestion (yet)


I was going to say the same. Jtreg has the concept of "java options" and 
"vm options". We have had a fair share of bugs and wasted time when 
tests have been using the "vm options" part (VM_OPTIONS, 
test.vm.options, etc), and we've been moving away from using that way to 
pass options. I recently cleaned up some of this with:


8237111: LingeredApp should be started with getTestJavaOpts

Because of this, I would prefer if we used a name that doesn't include 
"VmOpts", because it's too alike the other concept. Some suggestions:

 startAppJavaOptions
 startAppUsingJavaOptions
 startAppWithJavaOptions
 startAppExactJavaOptions
 startAppJvmOptions

Thanks,
StefanK


Thanks,
-- Igor


On Mar 25, 2020, at 8:55 AM, Leonid Mesnik  wrote:

Hi

Could you please review following fix which change LingeredApp to prepend vm 
options to java/vm.test.opts when startApp is used and provide startAppVmOpts 
to override options completely.

The intention is to avoid issue like in this bug where test/jtreg options were 
ignored by tests. Also I fixed some tests where intention was to append vm 
options rather than to override them.

webrev: http://cr.openjdk.java.net/~lmesnik/8240698/webrev.00/

bug: https://bugs.openjdk.java.net/browse/JDK-8240698

Leonid

Re: RFR(XS) 8240906: Update ZGC ProblemList for serviceability/sa/TestJmapCoreMetaspace.java

[Stefan Karlsson]

Looks good.

StefanK

On 2020-03-18 05:52, Chris Plummer wrote:

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8240906

diff --git a/test/hotspot/jtreg/ProblemList-zgc.txt 
b/test/hotspot/jtreg/ProblemList-zgc.txt

--- a/test/hotspot/jtreg/ProblemList-zgc.txt
+++ b/test/hotspot/jtreg/ProblemList-zgc.txt
@@ -47,5 +47,5 @@
 serviceability/sa/TestJhsdbJstackLock.java 8220624   generic-all
 serviceability/sa/TestJhsdbJstackMixed.java 8220624   generic-all
 serviceability/sa/TestJmapCore.java 8220624   generic-all
-serviceability/sa/TestJmapCoreMetaspace.java 8219443 generic-all
+serviceability/sa/TestJmapCoreMetaspace.java 8220624 generic-all
 serviceability/sa/sadebugd/DebugdConnectTest.java 8220624 generic-all

8219443 [1] was closed as a dup of 8219405 [2], which is a very 
intermittent bug that occurs even without ZGC, so should not be used 
to problem list this test for ZGC. However it should be ZGC problem 
listed due to 8220624 [3] just like TestJmapCore.java is.


[1] https://bugs.openjdk.java.net/browse/JDK-8219443
[2] https://bugs.openjdk.java.net/browse/JDK-8219405
[3] https://bugs.openjdk.java.net/browse/JDK-8220624

thanks,

Chris

Re: RFR(XS): 8239916 - SA: delete dead code in jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ObjectHeap.java

[Stefan Karlsson]

Looks good. This is left-overs from the CMS removal.

StefanK

On 2020-02-25 19:02, Chris Plummer wrote:

Adding hotspot-gc-dev.

Chris

On 2/25/20 2:21 AM, linzang(臧琳) wrote:

Hi,
    Please review the following change:
    Bugs: https://bugs.openjdk.java.net/browse/JDK-8239916
    webrev: http://cr.openjdk.java.net/~lzang/8239916/webrev/

Thanks,
Lin

Re: [15] RFR 8238633: JVMTI heap walk should consult GC for marking oops

[Stefan Karlsson]

Hi Zhengyu,

On 2020-02-17 15:51, Zhengyu Gu wrote:

Hi Stefan,

Thanks for the review and suggestions, updated accordingly:

http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.01/


Thanks for moving the code. I think this looks good.

If you're up for it, I have a couple of style change suggestions:

1) ObjectMarker uses two verbs to describe the same thing: "mark" and 
"visit". I propose that we only use "mark" in ObjectMarker and leave the 
usage of "visited" to the Jvmti code.


2) Some updates to odd whitespaces

3) Using forward declarations in Shenandoah code.

I've bundled those changes into webrevs:

https://cr.openjdk.java.net/~stefank/8238633/webrev.01.delta
https://cr.openjdk.java.net/~stefank/8238633/webrev.01

Regarding performance testing, the HeapWalkTests you used seems to use a 
very small heap. I think it would be good to redo the measurements on a 
larger heap. Could you take the HeapWalkTest and add a few GBs of small, 
linked objects?


Thank,
StefanK




---
Previously, the calls to 'mark' and 'visited' were inlineable, but 
now every GC has to take a virtual call when marking the objects. My 
guess is that this code is slow anyway, and that it doesn't matter 
too much, but did you measure the effect of that change with, for 
example, G1?


I did rough measurement, timing 
vmTestbase/nsk/jvmti/unit/heap/HeapWalkTests/TestDescription.java test.


If you know any tests/benchmarks I should measure, please let me know.

Thanks,

-Zhengyu



Thanks,
StefanK


Test:
   hotspot_gc
   vmTestbase_nsk_jdi
   vmTestbase_nsk_jvmti

Thanks,

-Zhengyu

Re: [15] RFR 8238633: JVMTI heap walk should consult GC for marking oops

[Stefan Karlsson]

Hi Zhengyu,

On 2020-02-07 16:53, Zhengyu Gu wrote:

Hi,

I would like purpose this change that allows GC to provide ObjectMarker 
during JVMTI heap walk.


Currently, JVMTI heap walk uses oop markword's 'marked' pattern to 
indicate 'visited' oop.


Unfortunately, it conflicts with Shenandoah, who uses the pattern to 
indicate 'forwarding'. When JVMTI heap walk occurs in some of 
Shenandoah's concurrent heap (e.g. concurrent evacuation or concurrent 
reference updating phases), it can result corrupted heap, as it tries to 
resolve a real oop header as a forwarding pointer.


This patch allows GC to provide ObjectMarker for JVMTI to track 
'visited' oop, and uses current implementation as default, so that, it 
has no impact to GCs other than Shenandoah, who provides its own 
implementation.


Bug: https://bugs.openjdk.java.net/browse/JDK-8238633
Webrev: http://cr.openjdk.java.net/~zgu/JDK-8238633/webrev.00/index.html




Would you mind if I asked you to move the object marker code to its own 
objectMarker.hpp/cpp files?


---
Another suggestion is to move the "marking" out of the visit() function, 
and renamed ObjectMarker::visted() to ObjectMarker::marked().


-  if (!ObjectMarker::visited(o)) {
-if (!visit(o)) {
+  if (!marker.object_marker()->visited(o)) {
+if (!visit(o, marker.object_marker())) {

Would become:
+  if (!marker.object_marker()->mark(o)) {
+if (!visit(o)) {

This assert would be unnecessary:
-bool VM_HeapWalkOperation::visit(oop o) {
+bool VM_HeapWalkOperation::visit(oop o, ObjectMarker* object_marker) {
   // mark object as visited
-  assert(!ObjectMarker::visited(o), "can't visit same object more than 
once");

-  ObjectMarker::mark(o);
+  assert(!object_marker->visited(o), "can't visit same object more than 
once");

+  object_marker->mark(o);

The name and comment would match:
-// return true if object is marked
-inline bool ObjectMarker::visited(oop o) {
-  return o->mark().is_marked();
-}

---
Previously, the calls to 'mark' and 'visited' were inlineable, but now 
every GC has to take a virtual call when marking the objects. My guess 
is that this code is slow anyway, and that it doesn't matter too much, 
but did you measure the effect of that change with, for example, G1?


Thanks,
StefanK


Test:
   hotspot_gc
   vmTestbase_nsk_jdi
   vmTestbase_nsk_jvmti

Thanks,

-Zhengyu

Re: RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Thanks. Created JDK-8237639.

StefanK

On 2020-01-22 10:50, David Holmes wrote:

Hi Stefan,

Thanks David. Would you accept it if I created a follow-up RFR to 
investigate if we could change order of the combined flags? 


Sure, no problem.

Thanks,
David

On 22/01/2020 6:58 pm, Stefan Karlsson wrote:

Hi David,

On 2020-01-22 05:28, David Holmes wrote:

Hi Stefan,

Thanks for tackling this.

On 22/01/2020 12:58 am, Stefan Karlsson wrote:

Hi all,

Please review this patch to change our usages of LingeredApp and 
getVmOptions() to instead use getTestJavaOpts().


https://cr.openjdk.java.net/~stefank/8237111/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8237111

This issue was encountered by both Coleen and I, independently.

We have two ways to pass JVM flags to jtreg. They come with 
different names depending on the test layer (make, jtreg, test.lib 
etc):


1) Utils.getVmOptions(), -vmoptions, -Dtest.vm.options, VM_OPTIONS, ...

  Is passed to all JVMs (not only the one under test)

2) -javaoptions, -Dtest.java.options, JAVA_OPTIONS, TEST_JAVA_OPTS, ...

  Is passed to tested JVM

The problem is that mach5 uses the latter to propagate JVM flags, so 
when tests explicitly uses Utils.getVmOptions() they won't run with 
the specified flags.


The problem also arise if someone runs the following on the command 
line:
make -C ../build/fastdebug test 
TEST=test/hotspot/jtreg/serviceability/sa/DeadlockDetectionTest.java 
JTREG="JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:+UseZGC"


There's no Utils.getJavaOptions() function that fetches the (2) 
flags, but there is a Utils.getTestJavaOpts() function that fetches 
flags from both (1) and (2).


There's some odd history here and the addition of getTestJavaOpts 
seemed to fly under the radar. It was reviewed by "sla" but I can 
only find the RFR request on serviceability-dev in Nov 2013, with no 
actual review. So the tests using getVmOptions have been broken for a 
very long time. :(


The proposal is to stop using (and remove) Utils.getVmOptions() and 
instead use Utils.getTestJavaOpts(). This patch touches more than 
LingeredApp, so we should probably rename it.


Agreed - please change synopsis to be more encompassing.


Some details about the patch:
- LingeredApp.startApp() now runs with getTestJavaOpts().


Good.

- getVmOptions() returned a List and getTestJavaOpts() 
returns a String[]. I've adapted the code to use String[] instead.


Works for me, but many Java programmers tend to be fond of using 
Collections over arrays. This code originated in the JDK version of 
the test library. :)


I actually started changing the code to only use List, but 
that change was much larger and reaching into non-hotspot/svc domains. 
The tests that today is using getTestJavaOpts() are already adapted to 
work with String[].


I don't mind if this were changed to List.



- Changed the parameter list of LingeredApp so that we could use 
String..., and lower the amount of boiler plate code.


- Removed Utils.getVmOptions()


Okay. The raw property values are still available if anyone actually 
wanted to use them for some reason.


- Left Utils.getForwardVmOptions() for now, because replacing it 
requires changes that needs to be reviewed on other lists.


Agreed - is there an open issue for this? (I also don't like the name 
of this method as it doesn't get the "forward VM options" is creates 
them.)


I created a placeholder RFR: JDK-8237634.

I think they meant forward from launcher to JVM. If we figure out a 
better name, we change change it with JDK-8237634.




- Added appendTestJavaOpts and prependTestJavaOpts since the order 
is important to tests.


H. I can't see any need for appendTestJavaOpts - none of the 
tests using it now actually need it versus prependTestJavaOpts. To 
use appendTestJavaOpts you have to know for certain that nothing in 
an incoming flag will interfere with the flags you are deliberately 
setting. Given "last flag wins" then you would "always" want the 
explicit per-test flags to come after the incoming flags.


That could be the case, but I don't want to change the current order 
of flags and risk braking something. Seems like an easy fix to change 
this as a separate RFE.




- Left addTestJavaOpts for now, because replacing it requires 
changes that needs to be reviewed on other lists.


Okay.

- Excluded some ZGC SA tests, because now we actually run with ZGC 
when we ask for it.


Okay

- JMapHeapConfigTest.java is broken when (jlong)-1 is passed in a 
flag. This prevented ZGC from running, because we set MaxNewSize to 
max size_t. Apparently, someone had already noticed this problem 
with MaxMetaspaceSize and added this cryptic line:

// ignoring MaxMetaspaceSize

I did the same for MaxNewSize and created a bug report.


Okay

- There are two instances of LingeredApp. I fixed both and created 
an enhancement to combine the two classes.


Okay

- ClhsdbF

Re: RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Hi David,

On 2020-01-22 05:28, David Holmes wrote:

Hi Stefan,

Thanks for tackling this.

On 22/01/2020 12:58 am, Stefan Karlsson wrote:

Hi all,

Please review this patch to change our usages of LingeredApp and 
getVmOptions() to instead use getTestJavaOpts().


https://cr.openjdk.java.net/~stefank/8237111/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8237111

This issue was encountered by both Coleen and I, independently.

We have two ways to pass JVM flags to jtreg. They come with different 
names depending on the test layer (make, jtreg, test.lib etc):


1) Utils.getVmOptions(), -vmoptions, -Dtest.vm.options, VM_OPTIONS, ...

  Is passed to all JVMs (not only the one under test)

2) -javaoptions, -Dtest.java.options, JAVA_OPTIONS, TEST_JAVA_OPTS, ...

  Is passed to tested JVM

The problem is that mach5 uses the latter to propagate JVM flags, so 
when tests explicitly uses Utils.getVmOptions() they won't run with 
the specified flags.


The problem also arise if someone runs the following on the command line:
make -C ../build/fastdebug test 
TEST=test/hotspot/jtreg/serviceability/sa/DeadlockDetectionTest.java 
JTREG="JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:+UseZGC"


There's no Utils.getJavaOptions() function that fetches the (2) flags, 
but there is a Utils.getTestJavaOpts() function that fetches flags 
from both (1) and (2).


There's some odd history here and the addition of getTestJavaOpts seemed 
to fly under the radar. It was reviewed by "sla" but I can only find the 
RFR request on serviceability-dev in Nov 2013, with no actual review. So 
the tests using getVmOptions have been broken for a very long time. :(


The proposal is to stop using (and remove) Utils.getVmOptions() and 
instead use Utils.getTestJavaOpts(). This patch touches more than 
LingeredApp, so we should probably rename it.


Agreed - please change synopsis to be more encompassing.


Some details about the patch:
- LingeredApp.startApp() now runs with getTestJavaOpts().


Good.

- getVmOptions() returned a List and getTestJavaOpts() returns 
a String[]. I've adapted the code to use String[] instead.


Works for me, but many Java programmers tend to be fond of using 
Collections over arrays. This code originated in the JDK version of the 
test library. :)


I actually started changing the code to only use List, but that 
change was much larger and reaching into non-hotspot/svc domains. The 
tests that today is using getTestJavaOpts() are already adapted to work 
with String[].


I don't mind if this were changed to List.



- Changed the parameter list of LingeredApp so that we could use 
String..., and lower the amount of boiler plate code.


- Removed Utils.getVmOptions()


Okay. The raw property values are still available if anyone actually 
wanted to use them for some reason.


- Left Utils.getForwardVmOptions() for now, because replacing it 
requires changes that needs to be reviewed on other lists.


Agreed - is there an open issue for this? (I also don't like the name of 
this method as it doesn't get the "forward VM options" is creates them.)


I created a placeholder RFR: JDK-8237634.

I think they meant forward from launcher to JVM. If we figure out a 
better name, we change change it with JDK-8237634.




- Added appendTestJavaOpts and prependTestJavaOpts since the order is 
important to tests.


H. I can't see any need for appendTestJavaOpts - none of the tests 
using it now actually need it versus prependTestJavaOpts. To use 
appendTestJavaOpts you have to know for certain that nothing in an 
incoming flag will interfere with the flags you are deliberately 
setting. Given "last flag wins" then you would "always" want the 
explicit per-test flags to come after the incoming flags.


That could be the case, but I don't want to change the current order of 
flags and risk braking something. Seems like an easy fix to change this 
as a separate RFE.




- Left addTestJavaOpts for now, because replacing it requires changes 
that needs to be reviewed on other lists.


Okay.

- Excluded some ZGC SA tests, because now we actually run with ZGC 
when we ask for it.


Okay

- JMapHeapConfigTest.java is broken when (jlong)-1 is passed in a 
flag. This prevented ZGC from running, because we set MaxNewSize to 
max size_t. Apparently, someone had already noticed this problem with 
MaxMetaspaceSize and added this cryptic line:

// ignoring MaxMetaspaceSize

I did the same for MaxNewSize and created a bug report.


Okay

- There are two instances of LingeredApp. I fixed both and created an 
enhancement to combine the two classes.


Okay

- ClhsdbFlags.runAllTypesTest used to *append* getVmOptions(). This 
started failing when I changed to getTestJavaOpts() because in some 
configs we override some of the flags in the test. I fixed it by 
*prepending* the getTestJavaOpts().


Okay. This reinforces my point above :)

Tested with various tiers, but not on the absolute

Re: RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Hi Chris,

On 2020-01-21 20:52, Chris Plummer wrote:

Hi Stefan,

Can you explain the un-commenting of the code in JpsHelper.java?


The comment was not removed, but moved over to testVmArgs.

The reason for the change is that I need getVmArgs() to return a 
String[] instead of List. So, I initialized testVmArgs with an 
array of the final sized, and then lazily initialize the runtime data.




Copyrights need updating.

Other than that it looks good.


Thanks for reviewing,
StefanK




thanks,

Chris

On 1/21/20 6:58 AM, Stefan Karlsson wrote:

Hi all,

Please review this patch to change our usages of LingeredApp and 
getVmOptions() to instead use getTestJavaOpts().


https://cr.openjdk.java.net/~stefank/8237111/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8237111

This issue was encountered by both Coleen and I, independently.

We have two ways to pass JVM flags to jtreg. They come with different 
names depending on the test layer (make, jtreg, test.lib etc):


1) Utils.getVmOptions(), -vmoptions, -Dtest.vm.options, VM_OPTIONS, ...

 Is passed to all JVMs (not only the one under test)

2) -javaoptions, -Dtest.java.options, JAVA_OPTIONS, TEST_JAVA_OPTS, ...

 Is passed to tested JVM

The problem is that mach5 uses the latter to propagate JVM flags, so 
when tests explicitly uses Utils.getVmOptions() they won't run with 
the specified flags.


The problem also arise if someone runs the following on the command line:
make -C ../build/fastdebug test 
TEST=test/hotspot/jtreg/serviceability/sa/DeadlockDetectionTest.java 
JTREG="JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:+UseZGC"


There's no Utils.getJavaOptions() function that fetches the (2) flags, 
but there is a Utils.getTestJavaOpts() function that fetches flags 
from both (1) and (2).


The proposal is to stop using (and remove) Utils.getVmOptions() and 
instead use Utils.getTestJavaOpts(). This patch touches more than 
LingeredApp, so we should probably rename it.


Some details about the patch:
- LingeredApp.startApp() now runs with getTestJavaOpts().

- getVmOptions() returned a List and getTestJavaOpts() returns 
a String[]. I've adapted the code to use String[] instead.


- Changed the parameter list of LingeredApp so that we could use 
String..., and lower the amount of boiler plate code.


- Removed Utils.getVmOptions()

- Left Utils.getForwardVmOptions() for now, because replacing it 
requires changes that needs to be reviewed on other lists.


- Added appendTestJavaOpts and prependTestJavaOpts since the order is 
important to tests.


- Left addTestJavaOpts for now, because replacing it requires changes 
that needs to be reviewed on other lists.


- Excluded some ZGC SA tests, because now we actually run with ZGC 
when we ask for it.


- JMapHeapConfigTest.java is broken when (jlong)-1 is passed in a 
flag. This prevented ZGC from running, because we set MaxNewSize to 
max size_t. Apparently, someone had already noticed this problem with 
MaxMetaspaceSize and added this cryptic line:

// ignoring MaxMetaspaceSize

I did the same for MaxNewSize and created a bug report.

- There are two instances of LingeredApp. I fixed both and created an 
enhancement to combine the two classes.


- ClhsdbFlags.runAllTypesTest used to *append* getVmOptions(). This 
started failing when I changed to getTestJavaOpts() because in some 
configs we override some of the flags in the test. I fixed it by 
*prepending* the getTestJavaOpts().


Tested with various tiers, but not on the absolute latest patch. Will 
run this through more testing when the review is done.


Thanks,
StefanK

RFR: 8237111: LingeredApp should be started with getTestJavaOpts

[Stefan Karlsson]

Hi all,

Please review this patch to change our usages of LingeredApp and 
getVmOptions() to instead use getTestJavaOpts().


https://cr.openjdk.java.net/~stefank/8237111/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8237111

This issue was encountered by both Coleen and I, independently.

We have two ways to pass JVM flags to jtreg. They come with different 
names depending on the test layer (make, jtreg, test.lib etc):


1) Utils.getVmOptions(), -vmoptions, -Dtest.vm.options, VM_OPTIONS, ...

 Is passed to all JVMs (not only the one under test)

2) -javaoptions, -Dtest.java.options, JAVA_OPTIONS, TEST_JAVA_OPTS, ...

 Is passed to tested JVM

The problem is that mach5 uses the latter to propagate JVM flags, so 
when tests explicitly uses Utils.getVmOptions() they won't run with the 
specified flags.


The problem also arise if someone runs the following on the command line:
make -C ../build/fastdebug test 
TEST=test/hotspot/jtreg/serviceability/sa/DeadlockDetectionTest.java 
JTREG="JAVA_OPTIONS=-XX:+UnlockExperimentalVMOptions -XX:+UseZGC"


There's no Utils.getJavaOptions() function that fetches the (2) flags, 
but there is a Utils.getTestJavaOpts() function that fetches flags from 
both (1) and (2).


The proposal is to stop using (and remove) Utils.getVmOptions() and 
instead use Utils.getTestJavaOpts(). This patch touches more than 
LingeredApp, so we should probably rename it.


Some details about the patch:
- LingeredApp.startApp() now runs with getTestJavaOpts().

- getVmOptions() returned a List and getTestJavaOpts() returns a 
String[]. I've adapted the code to use String[] instead.


- Changed the parameter list of LingeredApp so that we could use 
String..., and lower the amount of boiler plate code.


- Removed Utils.getVmOptions()

- Left Utils.getForwardVmOptions() for now, because replacing it 
requires changes that needs to be reviewed on other lists.


- Added appendTestJavaOpts and prependTestJavaOpts since the order is 
important to tests.


- Left addTestJavaOpts for now, because replacing it requires changes 
that needs to be reviewed on other lists.


- Excluded some ZGC SA tests, because now we actually run with ZGC when 
we ask for it.


- JMapHeapConfigTest.java is broken when (jlong)-1 is passed in a flag. 
This prevented ZGC from running, because we set MaxNewSize to max 
size_t. Apparently, someone had already noticed this problem with 
MaxMetaspaceSize and added this cryptic line:

// ignoring MaxMetaspaceSize

I did the same for MaxNewSize and created a bug report.

- There are two instances of LingeredApp. I fixed both and created an 
enhancement to combine the two classes.


- ClhsdbFlags.runAllTypesTest used to *append* getVmOptions(). This 
started failing when I changed to getTestJavaOpts() because in some 
configs we override some of the flags in the test. I fixed it by 
*prepending* the getTestJavaOpts().


Tested with various tiers, but not on the absolute latest patch. Will 
run this through more testing when the review is done.


Thanks,
StefanK

Re: RFR: 8226797: serviceability/tmtools/jstat/GcCapacityTest.java fails with Exception: java.lang.RuntimeException: OGCMN > OGCMX (min generation capacity > max generation capacity)

[Stefan Karlsson]

Hi Serguei,

On 2019-12-13 01:41, serguei.spit...@oracle.com wrote:

Hi Stefan,

It looks good to me.


Thanks for reviewing.



Sorry, I was on the meeting, wrote this email and forgot to push 'send' 
button.

Just now discovered that it has not been really sent. :(


No problem. I pushed this yesterday to make the JDK 14 fork cut-off.

Thanks,
StefanK



Thanks,
Serguei


On 12/12/19 07:23, Stefan Karlsson wrote:
In the interest to get this integrated before the RDP cut-off I'm 
going to push this ASAP. This has gone through tier1-tier3 testing.


StefanK

On 2019-12-12 13:01, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix a problem with unintialized values in 
our generation counters.


https://cr.openjdk.java.net/~stefank/8226797/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8226797

The jstat values NGCMN and OGCMN both return uninitialized values.

I stumbled upon this while creating a patch to remove the 
GenerationSpec class.


GenerationSpec::_min_size is never initialized, and then used to 
create the generations:


 case Generation::DefNew:
   return new DefNewGeneration(rs, _init_size, _min_size, 
_max_size);


 case Generation::MarkSweepCompact:
   return new TenuredGeneration(rs, _init_size, _min_size, 
_max_size, remset);


That in turn uses it to initialize the perf counters:
DefNewGeneration::DefNewGeneration(ReservedSpace rs,
    size_t initial_size,
    size_t min_size,
    size_t max_size,
    const char* policy)
...
   _gen_counters = new GenerationCounters("new", 0, 3,
   min_size, max_size, &_virtual_space);

I'm setting the value to _init_size, because it reflects how 
MinNewSize and MinOldSize relates to NewSize and OldSize.


Thanks,
StefanK

Re: RFR: 8226797: serviceability/tmtools/jstat/GcCapacityTest.java fails with Exception: java.lang.RuntimeException: OGCMN > OGCMX (min generation capacity > max generation capacity)

[Stefan Karlsson]

Thanks, Dan.

StefanK

On 2019-12-12 17:06, Daniel D. Daugherty wrote:

src/hotspot/share/gc/shared/generationSpec.hpp
     No comments.

test/hotspot/jtreg/serviceability/tmtools/jstat/utils/JstatGcCapacityResults.java 


     No comments.

Thumbs up.

Dan


On 12/12/19 10:23 AM, Stefan Karlsson wrote:
In the interest to get this integrated before the RDP cut-off I'm 
going to push this ASAP. This has gone through tier1-tier3 testing.


StefanK

On 2019-12-12 13:01, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix a problem with unintialized values in 
our generation counters.


https://cr.openjdk.java.net/~stefank/8226797/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8226797

The jstat values NGCMN and OGCMN both return uninitialized values.

I stumbled upon this while creating a patch to remove the 
GenerationSpec class.


GenerationSpec::_min_size is never initialized, and then used to 
create the generations:


 case Generation::DefNew:
   return new DefNewGeneration(rs, _init_size, _min_size, 
_max_size);


 case Generation::MarkSweepCompact:
   return new TenuredGeneration(rs, _init_size, _min_size, 
_max_size, remset);


That in turn uses it to initialize the perf counters:
DefNewGeneration::DefNewGeneration(ReservedSpace rs,
    size_t initial_size,
    size_t min_size,
    size_t max_size,
    const char* policy)
...
   _gen_counters = new GenerationCounters("new", 0, 3,
   min_size, max_size, &_virtual_space);

I'm setting the value to _init_size, because it reflects how 
MinNewSize and MinOldSize relates to NewSize and OldSize.


Thanks,
StefanK

Re: RFR: 8226797: serviceability/tmtools/jstat/GcCapacityTest.java fails with Exception: java.lang.RuntimeException: OGCMN > OGCMX (min generation capacity > max generation capacity)

[Stefan Karlsson]

In the interest to get this integrated before the RDP cut-off I'm going 
to push this ASAP. This has gone through tier1-tier3 testing.


StefanK

On 2019-12-12 13:01, Stefan Karlsson wrote:

Hi all,

Please review this patch to fix a problem with unintialized values in 
our generation counters.


https://cr.openjdk.java.net/~stefank/8226797/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8226797

The jstat values NGCMN and OGCMN both return uninitialized values.

I stumbled upon this while creating a patch to remove the GenerationSpec 
class.


GenerationSpec::_min_size is never initialized, and then used to create 
the generations:


     case Generation::DefNew:
   return new DefNewGeneration(rs, _init_size, _min_size, _max_size);

     case Generation::MarkSweepCompact:
   return new TenuredGeneration(rs, _init_size, _min_size, 
_max_size, remset);


That in turn uses it to initialize the perf counters:
DefNewGeneration::DefNewGeneration(ReservedSpace rs,
    size_t initial_size,
    size_t min_size,
    size_t max_size,
    const char* policy)
...
   _gen_counters = new GenerationCounters("new", 0, 3,
   min_size, max_size, &_virtual_space);

I'm setting the value to _init_size, because it reflects how MinNewSize 
and MinOldSize relates to NewSize and OldSize.


Thanks,
StefanK

RFR: 8226797: serviceability/tmtools/jstat/GcCapacityTest.java fails with Exception: java.lang.RuntimeException: OGCMN > OGCMX (min generation capacity > max generation capacity)

[Stefan Karlsson]

Hi all,

Please review this patch to fix a problem with unintialized values in 
our generation counters.


https://cr.openjdk.java.net/~stefank/8226797/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8226797

The jstat values NGCMN and OGCMN both return uninitialized values.

I stumbled upon this while creating a patch to remove the GenerationSpec 
class.


GenerationSpec::_min_size is never initialized, and then used to create 
the generations:


case Generation::DefNew:
  return new DefNewGeneration(rs, _init_size, _min_size, _max_size);

case Generation::MarkSweepCompact:
  return new TenuredGeneration(rs, _init_size, _min_size, 
_max_size, remset);


That in turn uses it to initialize the perf counters:
DefNewGeneration::DefNewGeneration(ReservedSpace rs,
   size_t initial_size,
   size_t min_size,
   size_t max_size,
   const char* policy)
...
  _gen_counters = new GenerationCounters("new", 0, 3,
  min_size, max_size, &_virtual_space);

I'm setting the value to _init_size, because it reflects how MinNewSize 
and MinOldSize relates to NewSize and OldSize.


Thanks,
StefanK

Re: RFR: 8229258: Rework markOop and markOopDesc into a simpler mark word value carrier

[Stefan Karlsson]

On 2019-08-16 00:59, Kim Barrett wrote:

On Aug 15, 2019, at 7:46 AM, Stefan Karlsson  wrote:

Thanks Kim, Roman, Dan and Coleen for reviews and feedback.

I rebased the patch, fixed more alignments, renamed the bug, and rerun the test 
through tier1-3.

https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03/

Could I get reviews for this version? I'd also like to ask others to at least 
partially look at this:


Here's my comments through webrev.valueMarkWord.03.

Looks good.  There are a couple of small fixes for which I don't need
a new webrev, which are listed first below.  Then there are some
broader items which could be addressed in followup improvements.

--
src/hotspot/share/oops/markOop.hpp
  109   template operator T();

My mistake in the earlier review comment.  Function should be const
qualified, e.g. that should be

   template operator T() const;



I added this after one of our earlier discussions. However, I don't 
think we need it (const or not). We already get sensible compiler errors 
without this function when we try to cast markWords to something else:


  void* p0 = m; 




  void* p1 = (void*)m; 




  int   i0 = m; 




  int   i1 = (int)m;

error: cannot convert ‘markWord’ to ‘void*’ in initialization 



   void* p0 = m; 




  ^ 




error: invalid cast from type ‘markWord’ to type ‘void*’ 



   void* p1 = (void*)m; 




 ^ 




error: cannot convert ‘markWord’ to ‘int’ in initialization 



   int   i0 = m; 




  ^ 




error: invalid cast from type ‘markWord’ to type ‘int’ 



   int   i1 = (int)m; 





The poisoned constructor seems to be unnecessary as well, now that we 
have simplified markWord. Without it, I get appropriate error messages 
when I try to create a markWord from a pointer:


error: invalid conversion from ‘void*’ to ‘uintptr_t’ {aka ‘long 
unsigned int’} [-fpermissive] 



   markWord p((void*)0x111); 




  ^~~~ 




note:   initializing argument 1 of ‘markWord::markWord(uintptr_t)’ 



   explicit markWord(uintptr_t value) : _value(value) { }

I've removed both of these.


--
src/hotspot/share/runtime/biasedLocking.cpp
  695  prototype.bias_epoch() == 
mark.bias_epoch())) {

I think one more leading space needs to be deleted to get proper
alignment here.  Or reformat this long and complex if control
expression.



OK. I followed the pre-existing alignment, but I can change it anyway.


--
src/hotspot/share/runtime/vframe.cpp
  244 // FIXME: mark is set but not used below.
  245 //Either the comment or the code is 
broken.

Is there a bug for this?



Created JDK-8229808.


--


The remainder seem like they could be followup improvements.

--
src/hotspot/share/oops/markOop.hpp
  138   static const uintptr_t zero = 0;

All occurrences of this are either
(1) markWord member initializater
(2) markWord variable initializer
(3) temporary markWord initializer

There don't appear to be any bare uses otherwise.  I think nicer is

static markWord zero() { return markWord(0); }

(For C++11: `static constexpr markWord zero = markWord(0);`)

This seems like it could be a followup improvement.



I had the same thought and then backed away from it, but I can't 
remember why. This is a small enough change, so I've gone through the 
few occurrences and cleaned it up.


I'll leave the rest of the comments below for follow-up RFEs.

This is the last few cleanups:
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04.delta/
http://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.04/

I ran extended testing on .03 (tier1-7 on linux), checked the markWord 
functions were inlined, and checked that the generated code for 
G1ParScanThreadState::copy_to_survivor_space was the same before and 
after the patch. So I intend to run tier1 testing on .04 and then push 
this patch.


Thanks,
StefanK


--

[This is also related to Coleen's comments about to_pointer.]

Looking at the changes, in order to reduce the amount of casting pixie
dust sprinkled on our code base, I now think markWord::to_pointer
should have a template overload, with the template parameter
designating the return type.  And I think the return type for the
non-template should be const qualified, and the template should handle
cv qualifiers.  Like so (I think, I haven't actually tested

Re: RFR: 8229258: Rework markOop and markOopDesc into a simpler mark word value carrier

[Stefan Karlsson]

Hi Roman,

On 2019-08-15 19:06, Roman Kennke wrote:

Hi Stefan,

I looked over the changes again. I like this much better, a huge
improvement over current state, and also better than your first
proposal. I also prefer the explicit value() calls.


Great!



I also built+tested Shenandoah GC again, seems all fine.

Didn't know that C++ has an 'explicit' specifier. Oh man.

Still seems to have foobared alignment (it was partly kaputted before
already):
src/hotspot/share/oops/oopsHierarchy.hpp


You're right. I removed one stray whitespace:
$ hg diff
diff --git a/src/hotspot/share/oops/oopsHierarchy.hpp 
b/src/hotspot/share/oops/oopsHierarchy.hpp

--- a/src/hotspot/share/oops/oopsHierarchy.hpp
+++ b/src/hotspot/share/oops/oopsHierarchy.hpp
@@ -46,7 +46,7 @@
 typedef class   instanceOopDesc*    instanceOop;
 typedef class   arrayOopDesc*   arrayOop;
 typedef class objArrayOopDesc*    objArrayOop;
-typedef class typeArrayOopDesc*    typeArrayOop;
+typedef class typeArrayOopDesc*   typeArrayOop;

 #else

I think the other indentation is done on purpose:
typedef class oopDesc*    oop;
typedef class   instanceOopDesc*    instanceOop;
typedef class   arrayOopDesc*   arrayOop;
typedef class objArrayOopDesc*    objArrayOop;
typedef class typeArrayOopDesc*   typeArrayOop;

to show the oops hierarchy.



Out of curiosity, what's with the changes in objectMonitor.inline.hpp to
access the markWord atomically?:

-inline markOop ObjectMonitor::header() const {
-  return _header;
+inline markWord ObjectMonitor::header() const {
+  return Atomic::load(&_header);
  }

I guess this is good (equal or stronger than before) but is there a
rationale behind these changes?


Ahh. Right. That was done to solve the problems I were having with 
volatiles. For example:
src/hotspot/share/runtime/objectMonitor.inline.hpp:38:10: error: binding 
reference of type 'const markWord&' to 'const volatile markWord' 
discards qualifiers

   return _header;

and:
src/hotspot/share/runtime/basicLock.hpp:40:74: error: implicit 
dereference will not access object of type ‘volatile markWord’ in 
statement [-Werror]
  void set_displaced_header(markWord header) { 
_displaced_header = header; }


Kim suggested that the fact that these fields were volatile was an 
indication that we should be doing some kind of atomic/ordered 
operation. By replacing these loads and stores with calls to the Atomic 
APIs, and providing the PrimitiveConversions::Translate 
specialization, we could solve that problem.




I say ship it!


Thanks a lot for reviewing this!

StefanK



Thanks,
Roman



Thanks Kim, Roman, Dan and Coleen for reviews and feedback.

I rebased the patch, fixed more alignments, renamed the bug, and rerun
the test through tier1-3.

https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03/

Could I get reviews for this version? I'd also like to ask others to at
least partially look at this:

1) Platform maintainers probably want to run this patch through their
build system.
2) SA maintainers (CC:ed serviceability-dev)
3) JVMCI maintainers

Thanks,
StefanK

On 2019-08-14 11:11, Roman Kennke wrote:

Am 14.08.19 um 01:26 schrieb Kim Barrett:

On Aug 12, 2019, at 12:19 PM, Stefan Karlsson
 wrote:

Hi Roman,

Kim helped me figuring out how to get past the volatile issues I had
with the class markWord { uintptr_t value; ... } version. So, I've
created a version with that:

https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.01/

I can go with either approach, so let me now what you all think.

I've finally had time to look at the first proposed change.

Comparing the first approach (an AllStatic MarkWord class and markWord
typedef'd to uintptr_t) vs the second approach (markWord is a thin
class wrapping around uintptr_t), I prefer the second.

* I think the markWord class provides better type safety. It still
involves too many casts sprinkled over the code base, but I think it
also provides a better basis for further cast reduction and
prevention.

* I think having one markWord class for the data and behavior is
better / more natural than having a markWord typedef for the data and
a MarkWord AllStatic class for the behaviour.

* I like that the markWord class eliminates the markWord vs MarkWord
homonyms, which I think will be annoying.

* The markWord class is a trivially copyable class, allowing it to be
efficiently passed around by value, so no disadvantage there.

I haven't found anything that I think argues for the first over the
second. Other folks might have different priorities or taste. I think
either is better than the status quo.

I'm still reviewing webrev.valueMarkWord.02, but so far haven't found
anything that makes me want to suggest backing off from that direction.

Note that the bug summary doesn't describe th

Re: RFR: 8229258: Rework markOop and markOopDesc into a simpler mark word value carrier

[Stefan Karlsson]

Thanks Kim, Roman, Dan and Coleen for reviews and feedback.

I rebased the patch, fixed more alignments, renamed the bug, and rerun 
the test through tier1-3.


https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03.delta/
https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.03/

Could I get reviews for this version? I'd also like to ask others to at 
least partially look at this:


1) Platform maintainers probably want to run this patch through their 
build system.

2) SA maintainers (CC:ed serviceability-dev)
3) JVMCI maintainers

Thanks,
StefanK

On 2019-08-14 11:11, Roman Kennke wrote:


Am 14.08.19 um 01:26 schrieb Kim Barrett:

On Aug 12, 2019, at 12:19 PM, Stefan Karlsson  
wrote:

Hi Roman,

Kim helped me figuring out how to get past the volatile issues I had with the 
class markWord { uintptr_t value; ... } version. So, I've created a version 
with that:

https://cr.openjdk.java.net/~stefank/8229258/webrev.valueMarkWord.01/

I can go with either approach, so let me now what you all think.

I've finally had time to look at the first proposed change.

Comparing the first approach (an AllStatic MarkWord class and markWord
typedef'd to uintptr_t) vs the second approach (markWord is a thin
class wrapping around uintptr_t), I prefer the second.

* I think the markWord class provides better type safety. It still
involves too many casts sprinkled over the code base, but I think it
also provides a better basis for further cast reduction and
prevention.

* I think having one markWord class for the data and behavior is
better / more natural than having a markWord typedef for the data and
a MarkWord AllStatic class for the behaviour.

* I like that the markWord class eliminates the markWord vs MarkWord
homonyms, which I think will be annoying.

* The markWord class is a trivially copyable class, allowing it to be
efficiently passed around by value, so no disadvantage there.

I haven't found anything that I think argues for the first over the
second. Other folks might have different priorities or taste. I think
either is better than the status quo.

I'm still reviewing webrev.valueMarkWord.02, but so far haven't found
anything that makes me want to suggest backing off from that direction.

Note that the bug summary doesn't describe the second approach.

+1 :-)

Roman

Re: RFR: 8227086: Use AS_NO_KEEPALIVE loads in HeapDumper

[Stefan Karlsson]

Thanks, Serguei.

StefanK

On 2019-07-02 17:57, serguei.spit...@oracle.com wrote:

Hi Stefan,

It looks good.

Thanks,
Serguei



On 7/2/19 06:53, Stefan Karlsson wrote:

Hi all,

Please review this patch to read objects with AS_NO_KEEPALIVE in the 
HeapDumper.


http://cr.openjdk.java.net/~stefank/8227086/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8227086

Found this while running some extra verification code through our 
barriers. This is one place where we use ON_UNKNOWN_OOP_REF without 
AS_NO_KEEPALIVE. The current code isn't wrong, but we could just as 
well use the more light-weight load barrier here.


Tested with tier 1-7

Thanks,
StefanK

Re: RFR: 8227086: Use AS_NO_KEEPALIVE loads in HeapDumper

[Stefan Karlsson]

Thanks, Kim.

StefanK

On 2019-07-03 00:11, Kim Barrett wrote:

On Jul 2, 2019, at 9:53 AM, Stefan Karlsson  wrote:

Hi all,

Please review this patch to read objects with AS_NO_KEEPALIVE in the HeapDumper.

http://cr.openjdk.java.net/~stefank/8227086/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8227086

Found this while running some extra verification code through our barriers. 
This is one place where we use ON_UNKNOWN_OOP_REF without AS_NO_KEEPALIVE. The 
current code isn't wrong, but we could just as well use the more light-weight 
load barrier here.

Tested with tier 1-7

Thanks,
StefanK

Looks good.

RFR: 8227086: Use AS_NO_KEEPALIVE loads in HeapDumper

[Stefan Karlsson]

Hi all,

Please review this patch to read objects with AS_NO_KEEPALIVE in the 
HeapDumper.


http://cr.openjdk.java.net/~stefank/8227086/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8227086

Found this while running some extra verification code through our 
barriers. This is one place where we use ON_UNKNOWN_OOP_REF without 
AS_NO_KEEPALIVE. The current code isn't wrong, but we could just as well 
use the more light-weight load barrier here.


Tested with tier 1-7

Thanks,
StefanK

Re: RFR: 8224479: New diagnostic command: VM.get_flag

[Stefan Karlsson]

Hi Thomas,

On 2019-05-21 12:37, Thomas Stüfe wrote:
I think this is useful. 


Thanks.

I have a vague preference for reusing VM.flags
though - giving it the option to only print one flag - instead of adding 
a new command.


So, we would have three different modes for VM.flags:
$ jcmd HelloSleep VM.flags
371:
-XX:CICompilerCount=15 -XX:ConcGCThreads=6 
-XX:G1ConcRefinementThreads=23 -XX:G1HeapRegionSize=4194304 
-XX:GCDrainStackTargetSize=64 -XX:InitialHeapSize=2113929216 
-XX:MarkStackSize=4194304 -XX:MaxHeapSize=32178700288 
-XX:MaxNewSize=19306381312 -XX:MinHeapDeltaBytes=4194304 
-XX:NonNMethodCodeHeapSize=8182140 -XX:NonProfiledCodeHeapSize=121738050 
-XX:ProfiledCodeHeapSize=121738050 -XX:ReservedCodeCacheSize=251658240 
-XX:+SegmentedCodeCache -XX:+UseCompressedClassPointers 
-XX:+UseCompressedOops -XX:+UseFastUnorderedTimeStamps -XX:+UseG1GC


$ /localhome/java/jdk9/bin/jcmd HelloSleep VM.flags -all
371:
[Global flags]
ccstrlist AOTLibrary   = 
  {product} {default}
  int ActiveProcessorCount = -1 
   {product} {default}
uintx AdaptiveSizeDecrementScaleFactor = 4 
   {product} {default}
uintx AdaptiveSizeMajorGCDecayTimeScale= 10 
   {product} {default}
uintx AdaptiveSizePolicyCollectionCostMargin   = 50 
   {product} {default}


and a new mode:
$ /localhome/java/jdk9/bin/jcmd HelloSleep VM.flags -name=UseSerialGC
371:
 bool UseSerialGC  = false 
   {product} {default}


Let's see if anyone else has some feedback regarding this.

Thanks,
StefanK



Just my 5c

.. Thomas

On Tue, May 21, 2019, 12:14 Stefan Karlsson <mailto:stefan.karls...@oracle.com>> wrote:


Hi all,

Please review this patch to introduce a new diagnostic command:
VM.get_flag.

http://cr.openjdk.java.net/~stefank/8224479/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8224479

Today we have:

- VM.set_flag - which allows the user to set a manageable flag
- VM.flags - which allows the user to print all set flags or print
similar output as -XX:+PrintFlagsFinal

I propose that we add a new command to print the value of one flag.

Output from help:
==
$ jcmd HelloSleep help VM.get_flag
1060:
VM.get_flag
Prints the value of a VM flag option.

Impact: Low

Permission: java.lang.management.ManagementPermission(monitor)

Syntax : VM.get_flag  

Arguments:
         flag name :  The name of the flag we want to get (STRING,
no default value)
==

Output from usage:
==
$ jcmd HelloSleep VM.get_flag UseSerialGC
1060:
false
==

I'll create a CSR if others also thinks this is a worthwhile feature.

Thanks,
StefanK

RFR: 8224479: New diagnostic command: VM.get_flag

[Stefan Karlsson]

Hi all,

Please review this patch to introduce a new diagnostic command: VM.get_flag.

http://cr.openjdk.java.net/~stefank/8224479/webrev.01/
https://bugs.openjdk.java.net/browse/JDK-8224479

Today we have:

- VM.set_flag - which allows the user to set a manageable flag
- VM.flags - which allows the user to print all set flags or print 
similar output as -XX:+PrintFlagsFinal


I propose that we add a new command to print the value of one flag.

Output from help:
==
$ jcmd HelloSleep help VM.get_flag
1060:
VM.get_flag
Prints the value of a VM flag option.

Impact: Low

Permission: java.lang.management.ManagementPermission(monitor)

Syntax : VM.get_flag  

Arguments:
flag name :  The name of the flag we want to get (STRING, no default 
value)
==

Output from usage:
==
$ jcmd HelloSleep VM.get_flag UseSerialGC
1060:
false
==

I'll create a CSR if others also thinks this is a worthwhile feature.

Thanks,
StefanK

Re: RFR (XXXS): 8221584: SIGSEGV in os::PlatformEvent::unpark() in JvmtiRawMonitor::raw_exit while posting method exit event

[Stefan Karlsson]

Looks good!

StefanK

On 2019-04-08 03:49, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8221584
webrev: http://cr.openjdk.java.net/~dholmes/8221584/webrev/

I'm really just sponsoring this fix as the problem was diagnozed by 
Robbin Ehn and Stefan Karlsson - thanks guys! :) So they are the 
contributors and I'm already one Reviewer.


There's a missing loadstore barrier between extracting the ParkEvent 
from an ObjectWaiter node, and setting the node's TState to allow the 
the entering thread to proceed. It seems our recent update to gcc 8.2 
resulted in the compiler reordering those two actions, meaning that 
the Objectwaiter pointer could now be pointing into a stack location 
with random contents. That might manifest as a SEGV or we may treat 
random memory as a pthread_mutex_t and get an EINVAL (or potentially 
other errors) on pthread_mutex_lock.


Testing: mach5 tiers 1-3 (sanity - the added barrier can't break 
anything)


Thanks,
David