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

[Coleen Phillimore]

On Wed, 27 Apr 2022 14:24:20 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 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

I've reviewed the new files in Hotspot runtime and the cpu directories, as well 
as the changes to existing files in runtime, oops, utilities, interpreter and 
classfile.
I'm happy to approve this PR. Thank you to everyone for their hard work in 
reviewing this code, and well done to @pron and @AlanBateman and all the loom 
team for bringing us this significant and exciting new feature for Java!

src/hotspot/share/prims/jvmtiEnvBase.cpp line 2388:

> 2386: }
> 2387: 
> 2388: void

@sspitsyn We should clean up this aberrant style of putting the return type on 
the line before the rest of the function declaration after this integration.  
It looks so strange.  I noticed that it's pre-existing for other functions in 
this file.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8177107: Reduce memory footprint of java.lang.reflect.Constructor/Method

[Coleen Phillimore]

On Mon, 4 Apr 2022 09:58:35 GMT, Claes Redestad  wrote:

> As an alternative to #7667 I took a look at injecting an empty class array 
> from the VM. Turns out we already do this for exception types - see 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/oops/method.cpp#L918
>  - and we can do similarly for the parameter types array. We still need to 
> parse the signature for the return type, though.
> 
> I've verified by dumping and inspecting heaps that this means we are not 
> allocating extra `Class[]` on `Method` reflection.

Looks good.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v3]

[Coleen Phillimore]

On Wed, 19 Jan 2022 05:44:10 GMT, Ioi Lam  wrote:

>> src/hotspot/share/cds/heapShared.cpp line 433:
>> 
>>> 431:   oop mirror = k->java_mirror();
>>> 432:   int i = 0;
>>> 433:   for (JavaFieldStream fs(k); !fs.done(); fs.next()) {
>> 
>> This seems like it should also use InstanceKlass::do_local_static_fields.
>
> Converting this to InstanceKlass::do_nonstatic_fields() is difficult because 
> the loop body references 7 different variables declared outside of the loop. 
> 
> One thing I tried is to add a new version of do_nonstatic_fields2() that 
> supports C++ lambdas. You can see my experiment from here: 
> 
> https://github.com/openjdk/jdk/compare/master...iklam:lambda-for-instanceklass-do_local_static_fields2?expand=1
> 
> I changed all my new code to use the do_nonstatic_fields2() function with 
> lambda.

Ok, if it requires lambdas and additional change, never mind then.

-

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

Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v6]

[Coleen Phillimore]

On Wed, 23 Feb 2022 04:15:28 GMT, Ioi Lam  wrote:

>> **Background:**
>> 
>> In the Java Language, Enums can be tested for equality, so the constants in 
>> an Enum type must be unique. Javac compiles an enum declaration like this:
>> 
>> 
>> public enum Day {  SUNDAY, MONDAY ... } 
>> 
>> 
>> to
>> 
>> 
>> public class Day extends java.lang.Enum {
>> public static final SUNDAY = new Day("SUNDAY");
>> public static final MONDAY = new Day("MONDAY"); ...
>> }
>> 
>> 
>> With CDS archived heap objects, `Day::` is executed twice: once 
>> during `java -Xshare:dump`, and once during normal JVM execution. If the 
>> archived heap objects references one of the Enum constants created at dump 
>> time, we will violate the uniqueness requirements of the Enum constants at 
>> runtime. See the test case in the description of 
>> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
>> 
>> **Fix:**
>> 
>> During -Xshare:dump, if we discovered that an Enum constant of type X is 
>> archived, we archive all constants of type X. At Runtime, type X will skip 
>> the normal execution of `X::`. Instead, we run 
>> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X 
>> that were saved at dump time.
>> 
>> This is safe as we know that `X::` has no observable side effect -- 
>> it only creates the constants of type X, as well as the synthetic value 
>> `X::$VALUES`, which cannot be observed until X is fully initialized.
>> 
>> **Verification:**
>> 
>> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for 
>> similar problems where the archived heap objects reference a static field 
>> that may be recreated at runtime. There are some manual steps involved, but 
>> I analyzed the potential problems found by the tool are they are all safe 
>> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details. 
>> An example trace of this tool can be found at 
>> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
>> 
>> **Testing:**
>> 
>> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   fixed whitespace

Sorry for the long delay. It's a big change, but a lot in debug so that's ok.  
Looks good.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8275731: CDS archived enums objects are recreated at runtime [v3]

[Coleen Phillimore]

On Sat, 11 Dec 2021 01:55:50 GMT, Ioi Lam  wrote:

>> **Background:**
>> 
>> In the Java Language, Enums can be tested for equality, so the constants in 
>> an Enum type must be unique. Javac compiles an enum declaration like this:
>> 
>> 
>> public enum Day {  SUNDAY, MONDAY ... } 
>> 
>> 
>> to
>> 
>> 
>> public class Day extends java.lang.Enum {
>> public static final SUNDAY = new Day("SUNDAY");
>> public static final MONDAY = new Day("MONDAY"); ...
>> }
>> 
>> 
>> With CDS archived heap objects, `Day::` is executed twice: once 
>> during `java -Xshare:dump`, and once during normal JVM execution. If the 
>> archived heap objects references one of the Enum constants created at dump 
>> time, we will violate the uniqueness requirements of the Enum constants at 
>> runtime. See the test case in the description of 
>> [JDK-8275731](https://bugs.openjdk.java.net/browse/JDK-8275731)
>> 
>> **Fix:**
>> 
>> During -Xshare:dump, if we discovered that an Enum constant of type X is 
>> archived, we archive all constants of type X. At Runtime, type X will skip 
>> the normal execution of `X::`. Instead, we run 
>> `HeapShared::initialize_enum_klass()` to retrieve all the constants of X 
>> that were saved at dump time.
>> 
>> This is safe as we know that `X::` has no observable side effect -- 
>> it only creates the constants of type X, as well as the synthetic value 
>> `X::$VALUES`, which cannot be observed until X is fully initialized.
>> 
>> **Verification:**
>> 
>> To avoid future problems, I added a new tool, CDSHeapVerifier, to look for 
>> similar problems where the archived heap objects reference a static field 
>> that may be recreated at runtime. There are some manual steps involved, but 
>> I analyzed the potential problems found by the tool are they are all safe 
>> (after the current bug is fixed). See cdsHeapVerifier.cpp for gory details. 
>> An example trace of this tool can be found at 
>> https://bugs.openjdk.java.net/secure/attachment/97242/enum_warning.txt
>> 
>> **Testing:**
>> 
>> Passed Oracle CI tiers 1-4. WIll run tier 5 as well.
>
> Ioi Lam 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 four additional commits since the 
> last revision:
> 
>  - Merge branch 'master' into 8275731-heapshared-enum
>  - added exclusions needed by "java -Xshare:dump -ea -esa"
>  - Comments from @calvinccheung off-line
>  - 8275731: CDS archived enums objects are recreated at runtime

I don't really know this code well enough to do a good code review.  I had some 
comments though.

src/hotspot/share/cds/cdsHeapVerifier.cpp line 165:

> 163: 
> 164: ResourceMark rm;
> 165: for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {

Can this call instead
void InstanceKlass::do_local_static_fields(void f(fieldDescriptor*, Handle, 
TRAPS), Handle mirror, TRAPS) {
and have this next few lines in the function?

src/hotspot/share/cds/cdsHeapVerifier.cpp line 254:

> 252:   InstanceKlass* ik = InstanceKlass::cast(k);
> 253:   for (JavaFieldStream fs(ik); !fs.done(); fs.next()) {
> 254: if (!fs.access_flags().is_static()) {

same here.  It only saves a couple of lines but then you can have the function 
outside this large function.

src/hotspot/share/cds/cdsHeapVerifier.hpp line 52:

> 50:   mtClassShared,
> 51:   HeapShared::oop_hash> _table;
> 52: 

Is this only used inside cdsHeapVerifier?  if so it should be in the .cpp file. 
There's also an ArchiveableStaticFieldInfo.  Not sure how they are related.

src/hotspot/share/cds/heapShared.cpp line 433:

> 431:   oop mirror = k->java_mirror();
> 432:   int i = 0;
> 433:   for (JavaFieldStream fs(k); !fs.done(); fs.next()) {

This seems like it should also use InstanceKlass::do_local_static_fields.

src/hotspot/share/cds/heapShared.cpp line 482:

> 480: copy_open_objects(open_regions);
> 481: 
> 482: CDSHeapVerifier::verify();

Should all this be DEBUG_ONLY ?

src/hotspot/share/cds/heapShared.hpp line 236:

> 234: oop _referrer;
> 235: oop _obj;
> 236: CachedOopInfo() :_subgraph_info(), _referrer(), _obj() {}

Should these be initialized to nullptr? does this do this?

-

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

Re: RFR: 8266936: Add a finalization JFR event [v17]

[Coleen Phillimore]

On Fri, 15 Oct 2021 09:27:54 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - renames
>  - spelling

Thanks for doing the renames.  Looks good!

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8266936: Add a finalization JFR event [v16]

[Coleen Phillimore]

On Wed, 13 Oct 2021 18:03:25 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR.

src/hotspot/share/jfr/support/jfrSymbolTable.cpp line 75:

> 73: 
> 74: JfrSymbolTable::JfrSymbolTable() :
> 75:   _symbol_table(new SymbolTable(this)),

I'm confused about which symbol table this is.  Is this the same SymbolTable as 
classfile/symbolTable.cpp? that instance is assumed to be a singleton.  Is this 
a different SymbolTable and can it have a different name (thought it was 
JfrSymbolTable).

src/hotspot/share/jfr/support/jfrSymbolTable.hpp line 68:

> 66:   template  class, 
> typename, size_t>
> 67:   friend class HashTableHost;
> 68:   typedef HashTableHost JfrSymbolTable> SymbolTable;

Oh here it is.  Since it's an enclosed type, can you rename it something 
simpler like Symbols and the other Strings?

-

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

Re: RFR: 8266936: Add a finalization JFR event [v10]

[Coleen Phillimore]

On Thu, 14 Oct 2021 21:58:42 GMT, Coleen Phillimore  wrote:

>> "Since you remove entries on unloading, I don't see any reason to have any 
>> concurrent cleanup."
>> Thank you, that is true. The only concurrent work required will be to grow 
>> the table.
>> 
>> "You do however need in the lookup code, some code that doesn't find the 
>> InstanceKlass if !ik->is_loader_alive()"
>> 
>> A precondition is that the code doing the lookup hold the 
>> ClassLoaderDataGraph_lock or is at a safepoint. Is that still the case? 
>> Would not purge_unloaded() take out the InstanceKlass from the table, as 
>> part of unloading, before !ik->is_loader_alive() is published to the outside 
>> world?
>
> Ok, I see - grow the table.
> 
> I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you 
> do.  The InstanceKlass is removed from your table during class unloading but 
> before that during concurrent class unloading, the class might not be alive 
> while you look at it and concurrent class unloading hasn't gotten around to 
> removing it yet.  Since you save classes regardless of CLD, you have to check 
> if it's alive.  See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for 
> example.  The CLDG_lock only keeps the graph from not getting modified, but 
> the classes in it might be dead.

That said, I don't see where you return an InstanceKlass in the table, which 
would need this check.  Not in class_unloading_do because this follows the 
_unloading list.

-

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

Re: RFR: 8266936: Add a finalization JFR event [v10]

[Coleen Phillimore]

On Mon, 13 Sep 2021 10:54:18 GMT, Markus Grönlund  wrote:

>> src/hotspot/share/services/finalizerService.cpp line 44:
>> 
>>> 42: _ik(ik),
>>> 43: _objects_on_heap(0),
>>> 44: _total_finalizers_run(0) {}
>> 
>> Is this hashtable for every InstanceKlass that defines a finalizer?  How 
>> many are there generally?  Why couldn't this be a simple hashtable like 
>> ResourceHashtable (soon to be renamed) which you can write in only a few 
>> lines of code?
>
> This hashtable holds a FinalizerEntry for every InstanceKlass that provides a 
> non-empty finalizer method and have allocated at least one object. How many 
> there are in general depends on the application. A ResourceHashtable does not 
> have the concurrency property required, as lookups and inserts will happen as 
> part of object allocation.

ok.

>> src/hotspot/share/services/finalizerService.cpp line 331:
>> 
>>> 329:   }
>>> 330:   Thread* const thread = Thread::current();
>>> 331:   // We use current size
>> 
>> Since you remove entries on unloading, I don't see any reason to have any 
>> concurrent cleanup.
>> You do however need in the lookup code, some code that doesn't find the 
>> InstanceKlass if !ik->is_loader_alive()
>
> "Since you remove entries on unloading, I don't see any reason to have any 
> concurrent cleanup."
> Thank you, that is true. The only concurrent work required will be to grow 
> the table.
> 
> "You do however need in the lookup code, some code that doesn't find the 
> InstanceKlass if !ik->is_loader_alive()"
> 
> A precondition is that the code doing the lookup hold the 
> ClassLoaderDataGraph_lock or is at a safepoint. Is that still the case? Would 
> not purge_unloaded() take out the InstanceKlass from the table, as part of 
> unloading, before !ik->is_loader_alive() is published to the outside world?

Ok, I see - grow the table.

I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you 
do.  The InstanceKlass is removed from your table during class unloading but 
before that during concurrent class unloading, the class might not be alive 
while you look at it and concurrent class unloading hasn't gotten around to 
removing it yet.  Since you save classes regardless of CLD, you have to check 
if it's alive.  See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for 
example.  The CLDG_lock only keeps the graph from not getting modified, but the 
classes in it might be dead.

-

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

Re: RFR: 8266936: Add a finalization JFR event [v11]

[Coleen Phillimore]

On Mon, 13 Sep 2021 17:12:49 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - remove rehashing and rely on default grow_hint for table resize
>  - mtStatistics

src/hotspot/share/jfr/periodic/jfrFinalizerStatisticsEvent.cpp line 26:

> 24: 
> 25: #include "precompiled.hpp"
> 26: #if INCLUDE_MANAGEMENT

With precompiled headers turned off, this gets a compilation error:
error: "INCLUDE_MANAGEMENT" is not
 defined, evaluates to 0 [-Werror=undef]

-

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

Re: RFR: 8266936: Add a finalization JFR event [v11]

[Coleen Phillimore]

On Mon, 13 Sep 2021 17:12:49 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - remove rehashing and rely on default grow_hint for table resize
>  - mtStatistics

src/hotspot/share/runtime/mutexLocker.cpp line 323:

> 321: 
> 322: #if INCLUDE_JFR
> 323:   def(JfrMsg_lock  , PaddedMonitor, leaf-1,  true,  
> _safepoint_check_always); // -1 because the ConcurrentHashTable resize lock 
> is leaf

The ConcurrentHashTable_lock is a lock that doesn't check for safepoints, so if 
you take this lock out while checking for safepoints, it should assert when 
called from a JavaThread, which makes me think it's not called from a 
JavaThread and should be _safepoint_check_never.
  _resize_lock =
new Mutex(Mutex::nosafepoint-2, "ConcurrentHashTableResize_lock",
  Mutex::_safepoint_check_never);
In my change, this is the ranking for ConcurrentHashTableResize_lock, so this 
should be nosafepoint-3 if  you check in after I do.

-

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

Re: RFR: 8266936: Add a finalization JFR event [v10]

[Coleen Phillimore]

On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - localize
>  - cleanup
>  - FinalizerStatistics

I have some general comments and questions about this code.

src/hotspot/share/services/classLoadingService.cpp line 80:

> 78: 
> 79: size_t ClassLoadingService::compute_class_size(InstanceKlass* k) {
> 80:   // lifted from ClassStatistics.do_class(Klass* k)

Can you remove this line? I think that function is gone now.

src/hotspot/share/services/finalizerService.cpp line 44:

> 42: _ik(ik),
> 43: _objects_on_heap(0),
> 44: _total_finalizers_run(0) {}

Is this hashtable for every InstanceKlass that defines a finalizer?  How many 
are there generally?  Why couldn't this be a simple hashtable like 
ResourceHashtable (soon to be renamed) which you can write in only a few lines 
of code?

src/hotspot/share/services/finalizerService.cpp line 114:

> 112: 
> 113: static inline void added() {
> 114:   set_atomic(&_count);

Why isn't Atomic::inc() good enough here? It's used everywhere else.

src/hotspot/share/services/finalizerService.cpp line 123:

> 121: static inline uintx hash_function(const InstanceKlass* ik) {
> 122:   assert(ik != nullptr, "invariant");
> 123:   return primitive_hash(ik);

If the hash function is primitive_hash, this hash is good enough to not need 
rehashing.  The only reason for the rehashing for symbol and string table was 
that the char* had a dumb hash that was faster but could be hacked, so it 
needed to become a different hashcode.  This doesn't need rehashing.

src/hotspot/share/services/finalizerService.cpp line 485:

> 483: void FinalizerService::purge_unloaded() {
> 484:   assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
> 485:   ClassLoaderDataGraph::classes_unloading_do(&on_unloading);

Since you remove entries on unloading, I don't see any reason to have any 
concurrent cleanup.
You do however need in the lookup code, some code that doesn't find the 
InstanceKlass if !ik->is_loader_alive()

-

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

Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

[Coleen Phillimore]

On Sun, 1 Aug 2021 08:17:08 GMT, Thomas Stuefe  wrote:

>> Short: this patch makes NMT available in custom-launcher scenarios and 
>> during gtests. It simplifies NMT initialization. It adds a lot of 
>> NMT-specific testing, cleans them up and makes them sideeffect-free.
>> 
>> -
>> 
>> NMT continues to be an extremely useful tool for SAP to tackle memory 
>> problems in the JVM.
>> 
>> However, NMT is of limited use due to the following restrictions:
>> 
>> - NMT cannot be used if the hotspot is embedded into a custom launcher 
>> unless the launcher actively cooperates. Just creating and invoking the JVM 
>> is not enough, it needs to do some steps prior to loading the hotspot. This 
>> limitation is not well known (nor, do I believe, documented). Many products 
>> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
>> problem limits NMT usefulness greatly since our VMs are often embedded into 
>> custom launchers and modifying every launcher is impossible.
>> - Worse, if that custom launcher links the libjvm *statically* there is just 
>> no way to activate NMT at all. This is the reason NMT cannot be used in the 
>> `gtestlauncher`.
>> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
>> and `-XX:Flags=`.
>> - The fact that NMT cannot be used in gtests is really a pity since it would 
>> allow us to both test NMT itself more rigorously and check for memory leaks 
>> while testing other stuff.
>> 
>> The reason for all this is that NMT initialization happens very early, on 
>> the first call to `os::malloc()`. And those calls happen already during 
>> dynamic C++ initialization - a long time before the VM gets around parsing 
>> arguments. So, regular VM argument parsing is too late to parse NMT 
>> arguments.
>> 
>> The current solution is to pass NMT arguments via a specially prepared 
>> environment variable: `NMT_LEVEL_=`. That environment 
>> variable has to be set by the embedding launcher, before it loads the 
>> libjvm. Since its name contains the PID, we cannot even set that variable in 
>> the shell before starting the launcher.
>> 
>> All that means that every launcher needs to especially parse and process the 
>> NMT arguments given at the command line (or via whatever method) and prepare 
>> the environment variable. `java` itself does this. This only works before 
>> the libjvm.so is loaded, before its dynamic C++ initialization. For that 
>> reason, it does not work if the launcher links statically against the 
>> hotspot, since in that case C++ initialization of the launcher and hotspot 
>> are folded into one phase with no possibility of executing code beforehand.
>> 
>> And since it bypasses argument handling in the VM, it bypasses a number of 
>> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
>> 
>> --
>> 
>> This patch fixes these shortcomings by making NMT late-initializable: it can 
>> now be initialized after normal VM argument parsing, like all other parts of 
>> the VM. This greatly simplifies NMT initialization and makes it work 
>> automagically for every third party launcher, as well as within our gtests.
>> 
>> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
>> we rule out just always having them (unacceptable in terms of memory 
>> overhead), there is no safe way to determine, in os::free(), if an 
>> allocation came from before or after NMT initialization ran, and therefore 
>> what to do with its malloc headers. For a more extensive explanation, please 
>> see the comment block `nmtPreInit.hpp` and the discussion with @kimbarrett 
>> and @zhengyu123 in the JBS comment section.
>> 
>> The heart of this patch is a new way to track early, pre-NMT-init 
>> allocations. These are tracked via a lookup table. This was a suggestion by 
>> Kim and it worked out well.
>> 
>> Changes in detail:
>> 
>> - pre-NMT-init handling:
>>  - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
>> handling. They contain a small global lookup table managing C-heap blocks 
>> allocated in the pre-NMT-init phase.
>>  - `os::malloc()/os::realloc()/os::free()` defer to this code before 
>> doing anything else.
>>  - Please see the extensive comment block at the start of 
>> `nmtPreinit.hpp` explaining the details.
>> 
>> - Changes to NMT:
>>  - Before, NMT initialization was spread over two phases, `initialize()` 
>> and `late_initialize()`. Those were merged into one and simplified - there 
>> is only one initialization now which happens after argument parsing.
>>  - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
>> simplify code, I changed NMT_unknown to be numerically 0. A new comment 
>> block in `nmtCommon.hpp` now clearly specifies what's what, including 
>> allowed level state transitions.
>>  - New utility functions to translate tracking level from/to strings 
>> added to `NMTUtil`
>>  - NMT has never been able to handle virtual memory al

Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

[Coleen Phillimore]

On Fri, 30 Jul 2021 09:32:22 GMT, Thomas Stuefe  wrote:

>> [Not a review, just a drive-by comment.]  This is a normal and idiomatic 
>> overload on the const-ness of `this`.
>
> To expand on Kim's answer. This is a way to solve const/nonconst problems 
> like https://github.com/openjdk/jdk/pull/4938/files#r679639391. 
> 
> You get a const version (suitably returning a write-protected pointer) which 
> the compiler chooses in const code, and a non-const version for non-const 
> code, and no awkward const-casts are needed from the outside.
> 
> In this case the implementation is simple enough that I just duplicated it; 
> were it more complex, I'd call one in terms of the other. We do this in other 
> places too, see e.g. `ResourceHashTable::lookup_node`.

This is ok. This was just new to me.

-

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

Re: RFR: JDK-8256844: Make NMT late-initializable [v2]

[Coleen Phillimore]

On Fri, 30 Jul 2021 09:44:57 GMT, Thomas Stuefe  wrote:

> Is that a real-life problem? Are there cases where the launcher would want to 
> live on if the JVM failed to load?

There are a lot of other reasons why the launcher couldn't live on if the JVM 
fails to load.  This was only one of them.  We used to think about this problem 
once but don't really think it's solveable.

-

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

Re: RFR: JDK-8256844: Make NMT late-initializable

[Coleen Phillimore]

On Thu, 22 Jul 2021 14:58:47 GMT, Thomas Stuefe  wrote:

> Short: this patch makes NMT available in custom-launcher scenarios and during 
> gtests. It simplifies NMT initialization. It adds a lot of NMT-specific 
> testing, cleans them up and makes them sideeffect-free.
> 
> -
> 
> NMT continues to be an extremely useful tool for SAP to tackle memory 
> problems in the JVM.
> 
> However, NMT is of limited use due to the following restrictions:
> 
> - NMT cannot be used if the hotspot is embedded into a custom launcher unless 
> the launcher actively cooperates. Just creating and invoking the JVM is not 
> enough, it needs to do some steps prior to loading the hotspot. This 
> limitation is not well known (nor, do I believe, documented). Many products 
> don't do this, e.g., you cannot use NMT with IntelliJ. For us at SAP this 
> problem limits NMT usefulness greatly since our VMs are often embedded into 
> custom launchers and modifying every launcher is impossible.
> - Worse, if that custom launcher links the libjvm *statically* there is just 
> no way to activate NMT at all. This is the reason NMT cannot be used in the 
> `gtestlauncher`.
> - Related to that is that we cannot pass NMT options via `JAVA_TOOL_OPTIONS` 
> and `-XX:Flags=`.
> - The fact that NMT cannot be used in gtests is really a pity since it would 
> allow us to both test NMT itself more rigorously and check for memory leaks 
> while testing other stuff.
> 
> The reason for all this is that NMT initialization happens very early, on the 
> first call to `os::malloc()`. And those calls happen already during dynamic 
> C++ initialization - a long time before the VM gets around parsing arguments. 
> So, regular VM argument parsing is too late to parse NMT arguments.
> 
> The current solution is to pass NMT arguments via a specially prepared 
> environment variable: `NMT_LEVEL_=`. That environment 
> variable has to be set by the embedding launcher, before it loads the libjvm. 
> Since its name contains the PID, we cannot even set that variable in the 
> shell before starting the launcher.
> 
> All that means that every launcher needs to especially parse and process the 
> NMT arguments given at the command line (or via whatever method) and prepare 
> the environment variable. `java` itself does this. This only works before the 
> libjvm.so is loaded, before its dynamic C++ initialization. For that reason, 
> it does not work if the launcher links statically against the hotspot, since 
> in that case C++ initialization of the launcher and hotspot are folded into 
> one phase with no possibility of executing code beforehand.
> 
> And since it bypasses argument handling in the VM, it bypasses a number of 
> argument processing ways, e.g., `JAVA_TOOL_OPTIONS`.
> 
> --
> 
> This patch fixes these shortcomings by making NMT late-initializable: it can 
> now be initialized after normal VM argument parsing, like all other parts of 
> the VM. This greatly simplifies NMT initialization and makes it work 
> automagically for every third party launcher, as well as within our gtests.
> 
> The glaring problem with late-initializing NMT is the NMT malloc headers. If 
> we rule out just always having them (unacceptable in terms of memory 
> overhead), there is no safe way to determine, in os::free(), if an allocation 
> came from before or after NMT initialization ran, and therefore what to do 
> with its malloc headers. For a more extensive explanation, please see the 
> comment block `nmtPreInit.hpp` and the discussion with @kimbarrett and 
> @zhengyu123 in the JBS comment section.
> 
> The heart of this patch is a new way to track early, pre-NMT-init 
> allocations. These are tracked via a lookup table. This was a suggestion by 
> Kim and it worked out well.
> 
> Changes in detail:
> 
> - pre-NMT-init handling:
>   - the new files `nmtPreInit.hpp/cpp` take case of NMT pre-init 
> handling. They contain a small global lookup table managing C-heap blocks 
> allocated in the pre-NMT-init phase.
>   - `os::malloc()/os::realloc()/os::free()` defer to this code before 
> doing anything else.
>   - Please see the extensive comment block at the start of 
> `nmtPreinit.hpp` explaining the details.
> 
> - Changes to NMT:
>   - Before, NMT initialization was spread over two phases, `initialize()` 
> and `late_initialize()`. Those were merged into one and simplified - there is 
> only one initialization now which happens after argument parsing.
>   - Minor changes were needed for the `NMT_TrackingLevel` enum - to 
> simplify code, I changed NMT_unknown to be numerically 0. A new comment block 
> in `nmtCommon.hpp` now clearly specifies what's what, including allowed level 
> state transitions.
>   - New utility functions to translate tracking level from/to strings 
> added to `NMTUtil`
>   - NMT has never been able to handle virtual memory allocations before 
> initialization, which is fine since os::reserve_memory() is not ca

Re: RFR: 8254598: StringDedupTable should use OopStorage

[Coleen Phillimore]

On Fri, 23 Apr 2021 19:48:47 GMT, Kim Barrett  wrote:

> Please review this change to the String Deduplication facility.  It is being
> changed to use OopStorage to hold weak references to relevant objects,
> rather than bespoke data structures requiring dedicated processing phases by
> supporting GCs.
> 
> (The Shenandoah update was contributed by Zhengyu Gu.)
> 
> This change significantly simplifies the interface between a given GC and
> the String Deduplication facility, which should make it much easier for
> other GCs to opt in.  However, this change does not alter the set of GCs
> providing support; currently only G1 and Shenandoah support string
> deduplication.  Adding support by other GCs will be in followup RFEs.
> 
> Reviewing via the diffs might not be all that useful for some parts, as
> several files have been essentially completely replaced, and a number of
> files have been added or eliminated.  The full webrev might be a better
> place to look.
> 
> The major changes are in gc/shared/stringdedup/* and in the supporting
> collectors, but there are also some smaller changes in other places, most
> notably classfile/{stringTable,javaClasses}.
> 
> This change is additionally labeled for review by core-libs, although there
> are no source changes there.  This change injects a byte field of bits into
> java.lang.String, using one of the previously unused padding bytes in that
> class.  (There were two unused bytes, now only one.)
> 
> Testing:
> mach5 tier1-2 with and without -XX:+UseStringDeduplication
> 
> Locally (linux-x64) ran all of the existing tests that use string
> deduplication with both G1 and Shenandoah.  Note that
> TestStringDeduplicationInterned.java is disabled for shenandoah, as it
> currently fails, for reasons I haven't figured out but suspect are test
> related.
> 
> - gc/stringdedup/   -- these used to be in gc/g1
> - runtime/cds/SharedStringsDedup.java
> - runtime/cds/appcds/cacheObject/DifferentHeapSizes.java
> - runtime/cds/appcds/sharedStrings/*
> 
> shenandoah-only:
> - gc/shenandoah/jvmti/TestHeapDump.java
> - gc/shenandoah/TestStringDedup.java
> - gc/shenandoah/TestStringDedupStress.java
> 
> Performance tested baseline, baseline + stringdedup, new with stringdedup,
> finding no significant differences.

I looked at the runtime code, which looks fine. I didn't read the GC code.

src/hotspot/share/classfile/javaClasses.inline.hpp line 77:

> 75: 
> 76: uint8_t* java_lang_String::flags_addr(oop java_string) {
> 77:   assert(_initialized, "Mut be initialized");

typo: must

src/hotspot/share/classfile/stringTable.cpp line 784:

> 782:   SharedStringIterator iter(f);
> 783:   _shared_table.iterate(&iter);
> 784: }

So with this change (somewhere below) do you no longer deduplicate strings from 
the shared string table? ie

// The CDS archive does not include the string deduplication table. Only the 
string
// table is saved in the archive. The shared strings from CDS archive need to be
// added to the string deduplication table before deduplication occurs. That is
// done in the beginning of the StringDedupThread (see 
StringDedupThread::do_deduplication()).
void StringDedupThread::deduplicate_shared_strings(StringDedupStat* stat) {
  StringDedupSharedClosure sharedStringDedup(stat);
  StringTable::shared_oops_do(&sharedStringDedup);
}
Asking @iklam to have a look then.

src/hotspot/share/runtime/globals.hpp line 1994:

> 1992: 
> \
> 1993:   product(uint64_t, StringDeduplicationHashSeed, 0, DIAGNOSTIC, 
> \
> 1994:   "Seed for the table hashing function; 0 requests computed 
> seed")  \

Should these two really be develop() options?

src/hotspot/share/runtime/mutexLocker.cpp line 239:

> 237: def(StringDedupQueue_lock  , PaddedMonitor, leaf,true,  
> _safepoint_check_never);
> 238: def(StringDedupTable_lock  , PaddedMutex  , leaf + 1,true,  
> _safepoint_check_never);
> 239:   }

Why weren't these duplicate definitions?  This is disturbing.

-

Marked as reviewed by coleenp (Reviewer).

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

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

[Coleen Phillimore]

On Thu, 8 Apr 2021 20:28:27 GMT, Igor Ignatyev  wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove exports from Graal module to jdk.aot
>
> Test changes look good to me.

There's a comment above
void VM_RedefineClasses::mark_dependent_code(InstanceKlass* ik) {
that should be rewritten, so I think we should just file an RFE to remove it 
afterwards.

-

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

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

[Coleen Phillimore]

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

I looked at the changes in oops, runtime, classfile and code directories.

-

Marked as reviewed by coleenp (Reviewer).

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

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

[Coleen Phillimore]

On Thu, 8 Apr 2021 15:23:52 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

void CodeCache::mark_for_evol_deoptimization(InstanceKlass* dependee) {
This function, its caller and the function in jvmtiRedefineClasses.cpp that 
calls it can be deleted also, but you can file a separate RFE for that if you 
want.

-

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

Re: RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package

[Coleen Phillimore]

On Thu, 11 Mar 2021 05:47:00 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review the patch which moves `ClassFileInstaller` class to 
> `jdk.test.lib.helpers` package? 
> to reduce changes in the tests, `ClassFileInstaller` in the default package 
> is kept w/ just `main` method that calls  `jdk.test.lib.helpers. 
> ClassFileInstaller::main`. 
> 
> from JBS:
>> ClassFileInstaller is in the default package hence it's impossible to use it 
>> directly by classes in any other package. although ClassFileInstaller is 
>> mainly used directly from jtreg test description, there are cases when one 
>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet 
>> to do. that these driver classes have to either be in a default package or 
>> use reflection, both approaches have drawbacks. 
>> 
>> to solve that, we can move ClassFileInstaller implementation to a package, 
>> and to avoid unneeded churn, keep package-less ClassFileInstaller class 
>> which will call package-full impl.
>>
>> the need for this patch has raised as part of 
>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
> 
> testing:
> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories 
> against `{macos,windows,linux}-x64`
> 
> Thanks,
> -- Igor

Looks good.  I looked at the RedefineClasses tests also.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

[Coleen Phillimore]

On Thu, 11 Feb 2021 12:44:54 GMT, Claes Redestad  wrote:

>> This patch moves some sanity checking done in ClassLoader.java to the 
>> corresponding endpoints in native or VM code.
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Consolidate verifyClassname and verifyFixClassname

This more limited cleanup looks good.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]

[Coleen Phillimore]

On Fri, 5 Feb 2021 03:00:05 GMT, David Holmes  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix comments and copyright.
>
> src/hotspot/share/classfile/javaClasses.cpp line 4415:
> 
>> 4413: 
>> 4414: // This field means that a security manager can be installed so we 
>> still have to
>> 4415: // populate the ProtectionDomainCacheTable.
> 
> No this field returns the installed SM if any. It doesn't tell you anything 
> about whether you can install a SM or not (though obviously if non-NULL then 
> you could).

// This field tells us that a security manager is installed.

How about I just put this.  I had a bug earlier that this explained to me but 
the allow_security_manager() also explains it.

> src/java.base/share/classes/java/lang/System.java line 163:
> 
>> 161: 
>> 162: // indicates if a security manager is possible
>> 163: // @implNote The HotSpot JVM hardcodes the value of NEVER.
> 
> You don't need this if the VM reads the value of NEVER.

ok.

-

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

Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]

[Coleen Phillimore]

> This change does not call up to Java for checkPackageAccess if the security 
> manager is NULL, but still saves the protection domain in the pd_set for that 
> dictionary entry.  If the option -Djava.security.manager=disallow is set, 
> that means that there will never be a security manager and the JVM code can 
> avoid saving the protection domains completely. 
> See the two functions java_lang_System::has_security_manager() and 
> java_lang_System::allow_security_manager() for details.
> Also deleted ProtectionDomainVerification because there's no use for this 
> option.
> 
> Tested with tier1 hotspot, jdk and langtools.
> and tier2-6.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  Fix comments and copyright.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2410/files
  - new: https://git.openjdk.java.net/jdk/pull/2410/files/296d0adb..7a2a3617

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=01-02

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

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

Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v2]

[Coleen Phillimore]

On Fri, 5 Feb 2021 01:45:58 GMT, David Holmes  wrote:

>> Coleen Phillimore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Add comment and read NEVER field from System
>
> src/hotspot/share/classfile/dictionary.cpp line 145:
> 
>> 143: #ifdef ASSERT
>> 144:   if (protection_domain == instance_klass()->protection_domain()) {
>> 145: MutexLocker ml(ProtectionDomainSet_lock, 
>> Mutex::_no_safepoint_check_flag);
> 
> By splitting the lock scope into two blocks you have lost the atomicity of 
> the entire action in a debug build, and now you check a potentially different 
> pd_set().

If the dictionary entry's class matches the protection domain, then it should 
never be added to the pd_set list.  So it doesn't need to be locked to check 
that.  It doesn't need atomicity.

> src/hotspot/share/classfile/javaClasses.cpp line 4406:
> 
>> 4404: }
>> 4405: 
>> 4406: // This field means that a security manager is never installed so we 
>> can completely
> 
> This field tells you whether a SM can be installed, and if not then you can 
> completely ...

updated with "we"

> src/hotspot/share/classfile/systemDictionary.cpp line 503:
> 
>> 501: } else {
>> 502:  log_debug(protectiondomain)("granted");
>> 503: }
> 
> Did you intend leaving this in?

Yes, why not? It's very useful in logging.

> test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java line 
> 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights 
>> reserved.
> 
> 2021 :)

I had a bug in my fix-copyrights script.

> test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java line 
> 47:
> 
>> 45: .shouldContain("[protectiondomain] Checking package access")
>> 46: .shouldContain("[protectiondomain] pd set count = #")
>> 47: .shouldHaveExitValue(0);
> 
> Minor nit: checking the exit value first can be more informative in case of a 
> crash.

The patterns I see have that last. (?)

-

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

Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v2]

[Coleen Phillimore]

> This change does not call up to Java for checkPackageAccess if the security 
> manager is NULL, but still saves the protection domain in the pd_set for that 
> dictionary entry.  If the option -Djava.security.manager=disallow is set, 
> that means that there will never be a security manager and the JVM code can 
> avoid saving the protection domains completely. 
> See the two functions java_lang_System::has_security_manager() and 
> java_lang_System::allow_security_manager() for details.
> Also deleted ProtectionDomainVerification because there's no use for this 
> option.
> 
> Tested with tier1 hotspot, jdk and langtools.
> and tier2-6.

Coleen Phillimore has updated the pull request incrementally with one 
additional commit since the last revision:

  Add comment and read NEVER field from System

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2410/files
  - new: https://git.openjdk.java.net/jdk/pull/2410/files/19ddd066..296d0adb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=00-01

  Stats: 25 lines in 3 files changed: 17 ins; 7 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2410.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2410/head:pull/2410

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

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

[Coleen Phillimore]

On Wed, 3 Feb 2021 19:49:30 GMT, Mandy Chung  wrote:

>> This patch moves some sanity checking done in ClassLoader.java to the 
>> corresponding endpoints in native or VM code.
>
> src/java.base/share/native/libjava/ClassLoader.c line 291:
> 
>> 289: }
>> 290: // disallow slashes in input, change '.' to '/'
>> 291: if (verifyFixClassname(clname)) {
> 
> perhaps we should replace all use of `fixClassname` with `verifyFixClassname` 
> and remove `fixClassname`.

This suggestion makes sense to me.  verifyClassName is only used once in 
Class.c passing false so you could remove that argument.
It's hard to see how fixClassName then verifyClassname is equivalent to 
verifyFixClassname but verifyFixClassname makes more sense than verifyClassname.
I think this return:
return (p != 0 && p - name == (ptrdiff_t)length);
implies a non-utf8 character was found?

-

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

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

[Coleen Phillimore]

On Thu, 4 Feb 2021 13:11:47 GMT, Coleen Phillimore  wrote:

>> src/java.base/share/native/libjava/ClassLoader.c line 291:
>> 
>>> 289: }
>>> 290: // disallow slashes in input, change '.' to '/'
>>> 291: if (verifyFixClassname(clname)) {
>> 
>> perhaps we should replace all use of `fixClassname` with 
>> `verifyFixClassname` and remove `fixClassname`.
>
> This suggestion makes sense to me.  verifyClassName is only used once in 
> Class.c passing false so you could remove that argument.
> It's hard to see how fixClassName then verifyClassname is equivalent to 
> verifyFixClassname but verifyFixClassname makes more sense than 
> verifyClassname.
> I think this return:
> return (p != 0 && p - name == (ptrdiff_t)length);
> implies a non-utf8 character was found?

Actually I think replacing fixClassName with verifyFixClassname will be awkward 
since the latter returns a value that's not checked in all the callers of 
fixClassName.  Maybe you could write fixClassName as:
void fixClassName() { verifyFixClassName(); with some assertion it passed? }

-

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

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM

[Coleen Phillimore]

On Wed, 3 Feb 2021 12:21:30 GMT, Claes Redestad  wrote:

> This patch moves some sanity checking done in ClassLoader.java to the 
> corresponding endpoints in native or VM code.

Changes requested by coleenp (Reviewer).

src/java.base/share/classes/java/lang/ClassLoader.java line 1259:

> 1257: Class findBootstrapClassOrNull(String name) {
> 1258: return findBootstrapClass(name);
> 1259: }

I'm confused why this would improve performance.  Wouldn't avoiding the 
transition between Java to the VM be good?  Or is checkName seldom false, so 
we're checking valid names for nothing?

-

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

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

[Coleen Phillimore]

On Tue, 26 Jan 2021 11:31:18 GMT, Anton Kozlov  wrote:

>> This could be a follow-up RFE.
>
> I assume a WXVerifier class that tracks W^X mode in debug mode and does 
> nothing in release mode. I've considered to do this, it's relates to small 
> inefficiencies, while this patch brings zero overhead (in release) for a 
> platform that does not need W^X. 
> * We don't need thread instance in release to call 
> `os::current_thread_enable_wx`. Having WXVerifier a part of the Thread will 
> require calling `Thread::current()` first and we could only hope for compiler 
> to optimize this out, not sure if it will happen at all. In some contexts the 
> Thread instance is available, in some it's not. 
> * An instance of the empty class (as WXVerifier will be in the release) will 
> occupy non-zero space in the Thread instance.
> 
> If such costs are negligible, I can do as suggested.

I really just want the minimal number of lines of code and hooks in thread.hpp. 
 You can still access it through the thread, just like these lines currently.  
Look at HandshakeState as an example.

-

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

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

[Coleen Phillimore]

On Mon, 25 Jan 2021 15:01:25 GMT, Anton Kozlov  wrote:

>> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:
>> 
>>> 85:   JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
>>> 86:   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
>>> 87:   Thread::WXWriteFromExecSetter wx_write;
>> 
>> Is this on every transition to VM from Native?  Would it be better to add to 
>> ThreadInVMfromNative like the ResetNoHandleMark is?
>
> I've tried to do something like this initially. The idea was to use Write in 
> VM state and Exec in Java and Native states. However, for example, JIT runs 
> in the Native state and needs Write access. So instead, W^X is managed on 
> entry and exit from VM code, in macros like JRT_ENTRY. Unfortunately, not 
> every JVM entry function is defined with an appropriate macro (at least for 
> now), so I had to add explicit W^X state management along with the explicit 
> java thread state, like here.

Yes, that's why I thought it should be added to the classes 
ThreadInVMfromNative, etc like:
class ThreadInVMfromNative : public ThreadStateTransition {
  ResetNoHandleMark __rnhm;

We can look at it with cleaning up the thread transitions RFE or as a 
follow-on.  If every line of ThreadInVMfromNative has to have one of these   
Thread::WXWriteVerifier __wx_write; people are going to miss them when 
adding the former.

-

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

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

[Coleen Phillimore]

On Mon, 25 Jan 2021 14:40:09 GMT, Coleen Phillimore  wrote:

>> Anton Kozlov has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - Address feedback for signature generators
>>  - Enable -Wformat-nonliteral back
>
> src/hotspot/share/runtime/thread.hpp line 915:
> 
>> 913:   verify_wx_state(WXExec);
>> 914: }
>> 915:   };
> 
> Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and 
> just add the class instance as a field in thread.hpp?

This could be a follow-up RFE.

-

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

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

[Coleen Phillimore]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

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

> 913:   verify_wx_state(WXExec);
> 914: }
> 915:   };

Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and just 
add the class instance as a field in thread.hpp?

-

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

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

[Coleen Phillimore]

On Sun, 24 Jan 2021 15:32:59 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:
> 
>  - Address feedback for signature generators
>  - Enable -Wformat-nonliteral back

src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:

> 85:   JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
> 86:   DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
> 87:   Thread::WXWriteFromExecSetter wx_write;

Is this on every transition to VM from Native?  Would it be better to add to 
ThreadInVMfromNative like the ResetNoHandleMark is?

-

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

RFR: 8257726: Make -XX:+StressLdcRewrite option a diagnostic option

[Coleen Phillimore]

See bug for details.  Tested:

$ java -XX:+StressLdcRewrite -version
Error: VM option 'StressLdcRewrite' is diagnostic and must be enabled via 
-XX:+UnlockDiagnosticVMOptions.
Error: The unlock option must precede 'StressLdcRewrite'.
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
$ java -XX:+UnlockDiagnosticVMOptions -XX:+StressLdcRewrite -version
openjdk version "16-internal" 2021-03-16
OpenJDK Runtime Environment (build 16-internal+0-2020-12-15-1356558.coleen...)
OpenJDK 64-Bit Server VM (build 16-internal+0-2020-12-15-1356558.coleen..., 
mixed mode, sharing)

Also, java/lang/instrument which has a test using StressLdcRewrite and tier1.

-

Commit messages:
 - 8257726: Make -XX:+StressLdcRewrite option a diagnostic option
 - 8257726: Make -XX:+StressLdcRewrite option a diagnostic option

Changes: https://git.openjdk.java.net/jdk/pull/1783/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1783&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257726
  Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1783.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1783/head:pull/1783

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v18]

[Coleen Phillimore]

On Tue, 10 Nov 2020 14:16:22 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the first incubation round 
>> of the foreign linker access API incubation
>> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
>> access support (see JEP 393 [2] and associated pull request [3]).
>> 
>> The main goal of this API is to provide a way to call native functions from 
>> Java code without the need of intermediate JNI glue code. In order to do 
>> this, native calls are modeled through the MethodHandle API. I suggest 
>> reading the writeup [4] I put together few weeks ago, which illustrates what 
>> the foreign linker support is, and how it should be used by clients.
>> 
>> Disclaimer: the pull request mechanism isn't great at managing *dependent* 
>> reviews. For this reasons, I'm attaching a webrev which contains only the 
>> differences between this PR and the memory access PR. I will be periodically 
>> uploading new webrevs, as new iterations come out, to try and make the life 
>> of reviewers as simple as possible.
>> 
>> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main 
>> architects of all the hotspot changes you see here, and without their help, 
>> the foreign linker support wouldn't be what it is today. As usual, a big 
>> thank to Paul Sandoz, who provided many insights (often by trying the bits 
>> first hand).
>> 
>> Thanks
>> Maurizio
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
>> 
>> Javadoc:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff (relative to [3]):
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254232
>> 
>> 
>> 
>> ### API Changes
>> 
>> The API changes are actually rather slim:
>> 
>> * `LibraryLookup`
>>   * This class allows clients to lookup symbols in native libraries; the 
>> interface is fairly simple; you can load a library by name, or absolute 
>> path, and then lookup symbols on that library.
>> * `FunctionDescriptor`
>>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
>> it is, at its core, an aggregate of memory layouts for the function 
>> arguments/return type. A function descriptor is used to describe the 
>> signature of a native function.
>> * `CLinker`
>>   * This is the real star of the show. A `CLinker` has two main methods: 
>> `downcallHandle` and `upcallStub`; the first takes a native symbol (as 
>> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` 
>> and returns a `MethodHandle` instance which can be used to call the target 
>> native symbol. The second takes an existing method handle, and a 
>> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a 
>> code stub allocated by the VM which acts as a trampoline from native code to 
>> the user-provided method handle. This is very useful for implementing 
>> upcalls.
>>* This class also contains the various layout constants that should be 
>> used by clients when describing native signatures (e.g. `C_LONG` and 
>> friends); these layouts contain additional ABI classfication information (in 
>> the form of layout attributes) which is used by the runtime to *infer* how 
>> Java arguments should be shuffled for the native call to take place.
>>   * Finally, this class provides some helper functions e.g. so that clients 
>> can convert Java strings into C strings and back.
>> * `NativeScope`
>>   * This is an helper class which allows clients to group together logically 
>> related allocations; that is, rather than allocating separate memory 
>> segments using separate *try-with-resource* constructs, a `NativeScope` 
>> allows clients to use a _single_ block, and allocate all the required 
>> segments there. This is not only an usability boost, but also a performance 
>> boost, since not all allocation requests will be turned into `malloc` calls.
>> * `MemorySegment`
>>   * Only one method added here - namely `handoff(NativeScope)` which allows 
>> a segment to be transferred onto an existing native scope.
>> 
>> ### Safety
>> 
>> The foreign linker API is intrinsically unsafe; many things can go wrong 
>> when requesting a native method handle. For instance, the description of the 
>> native signature might be wrong (e.g. have too many arguments) - and the 
>> runtime has, in the general case, no way to detect such mismatches. For 
>> these reasons, obtaining a `CLinker` instance is a *restricted* operation, 
>> which can be enabled by specifying the usual JDK property 
>> `-Dforeign.restricted=permit` (as it's the case for other restricted method 
>> in the foreign memory API).
>> 
>> ### Implementation changes
>> 
>> The Java changes associated with `LibraryLookup` are relative 
>> straightforward; the only interesting thing to note here is that library 

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]

[Coleen Phillimore]

On Mon, 9 Nov 2020 16:31:23 GMT, Jorn Vernee  wrote:

>> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 121:
>> 
>>> 119:  upcall_info.upcall_method.name, 
>>> upcall_info.upcall_method.sig,
>>> 120:  &args, thread);
>>> 121: }
>> 
>> This code shouldn't be in the cpu directory.  This should be in 
>> SharedRuntime or in jni.cpp.  It should have a JNI_ENTRY and not transition 
>> directly.  I don't know what AttachCurrentThreadAsDaemon does.
>
> Roger that.
> 
> We need the thread state transition though in case we get a random native 
> thread calling us.

yikes.  Does that work?

-

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]

[Coleen Phillimore]

On Thu, 5 Nov 2020 21:26:16 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the first incubation round 
>> of the foreign linker access API incubation
>> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
>> access support (see JEP 393 [2] and associated pull request [3]).
>> 
>> The main goal of this API is to provide a way to call native functions from 
>> Java code without the need of intermediate JNI glue code. In order to do 
>> this, native calls are modeled through the MethodHandle API. I suggest 
>> reading the writeup [4] I put together few weeks ago, which illustrates what 
>> the foreign linker support is, and how it should be used by clients.
>> 
>> Disclaimer: the pull request mechanism isn't great at managing *dependent* 
>> reviews. For this reasons, I'm attaching a webrev which contains only the 
>> differences between this PR and the memory access PR. I will be periodically 
>> uploading new webrevs, as new iterations come out, to try and make the life 
>> of reviewers as simple as possible.
>> 
>> A big thank to Jorn Vernee and Vladimir Ivanov - they are the main 
>> architects of all the hotspot changes you see here, and without their help, 
>> the foreign linker support wouldn't be what it is today. As usual, a big 
>> thank to Paul Sandoz, who provided many insights (often by trying the bits 
>> first hand).
>> 
>> Thanks
>> Maurizio
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
>> 
>> Javadoc:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff (relative to [3]):
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254232
>> 
>> 
>> 
>> ### API Changes
>> 
>> The API changes are actually rather slim:
>> 
>> * `LibraryLookup`
>>   * This class allows clients to lookup symbols in native libraries; the 
>> interface is fairly simple; you can load a library by name, or absolute 
>> path, and then lookup symbols on that library.
>> * `FunctionDescriptor`
>>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
>> it is, at its core, an aggregate of memory layouts for the function 
>> arguments/return type. A function descriptor is used to describe the 
>> signature of a native function.
>> * `CLinker`
>>   * This is the real star of the show. A `CLinker` has two main methods: 
>> `downcallHandle` and `upcallStub`; the first takes a native symbol (as 
>> obtained from `LibraryLookup`), a `MethodType` and a `FunctionDescriptor` 
>> and returns a `MethodHandle` instance which can be used to call the target 
>> native symbol. The second takes an existing method handle, and a 
>> `FunctionDescriptor` and returns a new `MemorySegment` corresponding to a 
>> code stub allocated by the VM which acts as a trampoline from native code to 
>> the user-provided method handle. This is very useful for implementing 
>> upcalls.
>>* This class also contains the various layout constants that should be 
>> used by clients when describing native signatures (e.g. `C_LONG` and 
>> friends); these layouts contain additional ABI classfication information (in 
>> the form of layout attributes) which is used by the runtime to *infer* how 
>> Java arguments should be shuffled for the native call to take place.
>>   * Finally, this class provides some helper functions e.g. so that clients 
>> can convert Java strings into C strings and back.
>> * `NativeScope`
>>   * This is an helper class which allows clients to group together logically 
>> related allocations; that is, rather than allocating separate memory 
>> segments using separate *try-with-resource* constructs, a `NativeScope` 
>> allows clients to use a _single_ block, and allocate all the required 
>> segments there. This is not only an usability boost, but also a performance 
>> boost, since not all allocation requests will be turned into `malloc` calls.
>> * `MemorySegment`
>>   * Only one method added here - namely `handoff(NativeScope)` which allows 
>> a segment to be transferred onto an existing native scope.
>> 
>> ### Safety
>> 
>> The foreign linker API is intrinsically unsafe; many things can go wrong 
>> when requesting a native method handle. For instance, the description of the 
>> native signature might be wrong (e.g. have too many arguments) - and the 
>> runtime has, in the general case, no way to detect such mismatches. For 
>> these reasons, obtaining a `CLinker` instance is a *restricted* operation, 
>> which can be enabled by specifying the usual JDK property 
>> `-Dforeign.restricted=permit` (as it's the case for other restricted method 
>> in the foreign memory API).
>> 
>> ### Implementation changes
>> 
>> The Java changes associated with `LibraryLookup` are relative 
>> straightforward; the only interesting thing to note here is that library 

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v15]

[Coleen Phillimore]

On Fri, 6 Nov 2020 21:47:42 GMT, Coleen Phillimore  wrote:

>> Maurizio Cimadamore has updated the pull request with a new target base due 
>> to a merge or a rebase. The pull request now contains 64 commits:
>> 
>>  - Merge branch '8254162' into 8254231_linker
>>  - Fix post-merge issues caused by 8219014
>>  - Merge branch 'master' into 8254162
>>  - Addess remaining feedback from @AlanBateman and @mrserb
>>  - Address comments from @AlanBateman
>>  - Fix typo in upcall helper for aarch64
>>  - Merge branch '8254162' into 8254231_linker
>>  - Merge branch 'master' into 8254162
>>  - Fix issues with derived buffers and IO operations
>>  - More 32-bit fixes for TestLayouts
>>  - ... and 54 more: 
>> https://git.openjdk.java.net/jdk/compare/a50fdd54...b38afb3f
>
> src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 81:
> 
>> 79: #endif
>> 80: 
>> 81:   Method* method = k->lookup_method(mname_sym, mdesc_sym);
> 
> This "method" appears unused.

This should be moved into javaClasses or common code.  resolve_or_null only 
resolves the class, it doesn't also call the initializer for the class so you 
shouldn't be able to call a static method on the class.

-

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

Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]

[Coleen Phillimore]

On Thu, 15 Oct 2020 17:08:28 GMT, Maurizio Cimadamore  
wrote:

>> This patch contains the changes associated with the first incubation round 
>> of the foreign linker access API incubation
>> (see JEP 389 [1]). This work is meant to sit on top of the foreign memory 
>> access support (see JEP 393 [2] and
>> associated pull request [3]).
>> The main goal of this API is to provide a way to call native functions from 
>> Java code without the need of intermediate
>> JNI glue code. In order to do this, native calls are modeled through the 
>> MethodHandle API. I suggest reading the
>> writeup [4] I put together few weeks ago, which illustrates what the foreign 
>> linker support is, and how it should be
>> used by clients.  Disclaimer: the pull request mechanism isn't great at 
>> managing *dependent* reviews. For this reasons,
>> I'm attaching a webrev which contains only the differences between this PR 
>> and the memory access PR. I will be
>> periodically uploading new webrevs, as new iterations come out, to try and 
>> make the life of reviewers as simple as
>> possible.  A big thank to Jorn Vernee and Vladimir Ivanov - they are the 
>> main architects of all the hotspot changes you
>> see here, and without their help, the foreign linker support wouldn't be 
>> what it is today. As usual, a big thank to
>> Paul Sandoz, who provided many insights (often by trying the bits first 
>> hand).  Thanks Maurizio
>> Webrev:
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev
>> 
>> Javadoc:
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html
>> 
>> Specdiff (relative to [3]):
>> 
>> http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html
>> 
>> CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254232
>> 
>> 
>> 
>> ### API Changes
>> 
>> The API changes are actually rather slim:
>> 
>> * `LibraryLookup`
>>   * This class allows clients to lookup symbols in native libraries; the 
>> interface is fairly simple; you can load a library
>> by name, or absolute path, and then lookup symbols on that library.
>> * `FunctionDescriptor`
>>   * This is an abstraction that is very similar, in spirit, to `MethodType`; 
>> it is, at its core, an aggregate of memory
>> layouts for the function arguments/return type. A function descriptor is 
>> used to describe the signature of a native
>> function.
>> * `CLinker`
>>   * This is the real star of the show. A `CLinker` has two main methods: 
>> `downcallHandle` and `upcallStub`; the first takes
>> a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a 
>> `FunctionDescriptor` and returns a
>> `MethodHandle` instance which can be used to call the target native 
>> symbol. The second takes an existing method handle,
>> and a `FunctionDescriptor` and returns a new `MemorySegment` 
>> corresponding to a code stub allocated by the VM which
>> acts as a trampoline from native code to the user-provided method 
>> handle. This is very useful for implementing upcalls.
>>* This class also contains the various layout constants that should be 
>> used by clients when describing native signatures
>>  (e.g. `C_LONG` and friends); these layouts contain additional ABI 
>> classfication information (in the form of layout
>>  attributes) which is used by the runtime to *infer* how Java arguments 
>> should be shuffled for the native call to take
>>  place.
>>   * Finally, this class provides some helper functions e.g. so that clients 
>> can convert Java strings into C strings and
>> back.
>> * `NativeScope`
>>   * This is an helper class which allows clients to group together logically 
>> related allocations; that is, rather than
>> allocating separate memory segments using separate *try-with-resource* 
>> constructs, a `NativeScope` allows clients to
>> use a _single_ block, and allocate all the required segments there. This 
>> is not only an usability boost, but also a
>> performance boost, since not all allocation requests will be turned into 
>> `malloc` calls.
>> * `MemorySegment`
>>   * Only one method added here - namely `handoff(NativeScope)` which allows 
>> a segment to be transferred onto an existing
>> native scope.
>> 
>> ### Safety
>> 
>> The foreign linker API is intrinsically unsafe; many things can go wrong 
>> when requesting a native method handle. For
>> instance, the description of the native signature might be wrong (e.g. have 
>> too many arguments) - and the runtime has,
>> in the general case, no way to detect such mismatches. For these reasons, 
>> obtaining a `CLinker` instance is
>> a *restricted* operation, which can be enabled by specifying the usual JDK 
>> property `-Dforeign.restricted=permit` (as
>> it's the case for other restricted method in the foreign memory API).  ### 
>> Implementation changes  The Java changes
>> associated with `LibraryLookup` are relative straightforward;

Re: RFR: 8246774: Record Classes (final) implementation [v3]

[Coleen Phillimore]

On Wed, 23 Sep 2020 03:34:29 GMT, Vicente Romero  wrote:

>> Co-authored-by: Vicente Romero 
>> Co-authored-by: Harold Seigel 
>> Co-authored-by: Jonathan Gibbons 
>> Co-authored-by: Brian Goetz 
>> Co-authored-by: Maurizio Cimadamore 
>> Co-authored-by: Joe Darcy 
>> Co-authored-by: Chris Hegarty 
>> Co-authored-by: Jan Lahoda 
>
> Vicente Romero has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Merge pull request #1 from ChrisHegarty/record-serial-tests
>
>Remove preview args from JDK tests
>  - Remove preview args from ObjectMethodsTest
>  - Remove preview args from record serialization tests

The classfile parser changes look good to me.

-

Marked as reviewed by coleenp (Reviewer).

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

Re: RFR: 8241390: 'Deadlock' with VM_RedefineClasses::lock_classes()

[Coleen Phillimore]

On Tue, 15 Sep 2020 16:43:01 GMT, Daniil Titov  wrote:

> The change fixes a 'deadlock' situation in VM_RedefineClasses::lock_classes() 
> when the current thread is in the middle
> of redefining the same class. The change is based on the fix [1] suggested in 
> the Jira issue [2] comment.
> [1] http://cr.openjdk.java.net/~jiangli/8241390/webrev.00/
> [2] https://bugs.openjdk.java.net/browse/JDK-8241390
> 
> Testing: :jdk_instrument, tier1-tier3, and tier5 tests pass.

Changes requested by coleenp (Reviewer).

test/jdk/java/lang/instrument/MakeAgentJAR.sh line 1:

> 1: #!/bin/sh

There are tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses that 
don't use shell scripts that are much
better.  Can you add this test using that framework instead?

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 159:

> 157: if (!cls->contains(def_ik)) {
> 158:   def_ik->set_is_being_redefined(false);
> 159: }

Ok, so adding the Klass to the thread local list for each recursion works like 
ref counting.  Can't think of a simpler
way to do this.  Looks good.

-

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

Re: RFR: 8244778: Archive full module graph in CDS [v3]

[Coleen Phillimore]

On Wed, 9 Sep 2020 18:46:33 GMT, Lois Foltan 
 wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Feedback from Coleen
>
> Thanks Ioi for addressing my review comments.   Overall, looks great!

Ok thanks!  So many emails ...

-

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

Re: RFR: 8244778: Archive full module graph in CDS

[Coleen Phillimore]

On Tue, 8 Sep 2020 15:59:33 GMT, Ioi Lam  wrote:

> This is the same patch as
> [8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/)
> published in
> [hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html).
> The rest of the review will continue on GitHub. I will add new commits to 
> respond to comments to the above e-mail.

Marked as reviewed by coleenp (Reviewer).

src/hotspot/share/classfile/classLoaderDataShared.cpp line 132:

> 130:   assert(loader_data != NULL, "must be");
> 131:   return loader_data;
> 132: }

This and other private functions should probably be a static function inside 
classLoaderDataShared.cpp.

src/hotspot/share/classfile/classLoaderDataShared.hpp line 28:

> 26: #define SHARE_CLASSFILE_CLASSLOADERDATASHARED_HPP
> 27:
> 28: #include "utilities/exceptions.hpp"

There's a memory/allStatic.hpp file now that this should include.

src/hotspot/share/classfile/modules.cpp line 495:

> 493:   }
> 494: }
> 495: #endif

Nit: can you add // INCLUDE_CDS_JAVA_HEAP

src/hotspot/share/classfile/classLoaderDataShared.cpp line 171:

> 169: }
> 170:
> 171: void ClassLoaderDataShared::serialize(class SerializeClosure* f) {

Why is there a 'class' keyword here?

-

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

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[coleen . phillimore]

On 3/30/20 5:54 AM, David Holmes wrote:
Sorry to jump in on this but it caught my eye though I may be missing 
a larger context ...


On 30/03/2020 7:30 pm, serguei.spit...@oracle.com wrote:

Hi Mandy,

I have just one comment so far.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03/src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp.frames.html 



  356   void add_classes(LoadedClassInfo* first_class, int 
num_classes, bool has_class_mirror_holder) {

  357 LoadedClassInfo** p_list_to_add_to;
  358 bool is_hidden = first_class->_klass->is_hidden();
  359 if (has_class_mirror_holder) {
  360   p_list_to_add_to = is_hidden ? &_hidden_weak_classes : 
&_anon_classes;

  361 } else {
  362   p_list_to_add_to = &_classes;
  363 }
  364 // Search tail.
  365 while ((*p_list_to_add_to) != NULL) {
  366   p_list_to_add_to = &(*p_list_to_add_to)->_next;
  367 }
  368 *p_list_to_add_to = first_class;
  369 if (has_class_mirror_holder) {
  370   if (is_hidden) {
  371 _num_hidden_weak_classes += num_classes;


Why does hidden imply weak here?


has_class_mirror_holder() implies weak.

Coleen


David
-


  372   } else {
  373 _num_anon_classes += num_classes;
  374   }
  375 } else {
  376   _num_classes += num_classes;
  377 }
  378   }

  Q1: I'm just curious, what happens if a cld has arrays of hidden 
classes?

  Is the bottom_klass always expected to be the first?


Thanks,
Serguei


On 3/26/20 16:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The 
main changes are in core-libs and hotspot runtime area.  Small 
changes are made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed 
and is in the finalized state (see specdiff and javadoc below for 
reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's 
point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not 
registered in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection. setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

   can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden 
class.
4. Field::setXXX method will throw IAE on a final field of a hidden 
class

   regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

   and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
   that holds the classes strongly referenced by its defining 
loader. There

   can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control
   check no longer throws LinkageError but instead it will throw IAE 
with
   a clear message if a class fails to resolve/validate the nest 
host declared

   in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
   and generate a bridge method to desuger a method reference to a 
protected

   method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError 
and intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidd

Re: RFR: JEP 359: Records (Preview) (full code)

[coleen . phillimore]

(resending to all the lists)

Hi, I've looked at the hotspot code again, and it looks good.  Nice 
work, Harold and Vincente!


Coleen


On 11/27/19 11:37 PM, Vicente Romero wrote:

Hi,

Please review the code for the records feature at [1]. This webrev 
includes all: APIs, runtime, compiler, serialization, javadoc, and 
more! Must of the code has been reviewed but there have been some 
changes since reviewers saw it. Also this is the first time an 
integral webrev is sent out for review. Last changes on top of my mind 
since last review iterations:


On the compiler implementation:
- it has been adapted to the last version of the language spec [2], as 
a reference the JVM spec is at [3]. This implied some changes in 
determining if a user defined constructor is the canonical or not. Now 
if a constructor is override-equivalent to a signature derived from 
the record components, then it is considered the canonical 
constructor. And any canonical constructor should satisfy a set of 
restrictions, see section 8.10.4 Record Constructor Declarations of 
the specification.

- It was also added a check to make sure that accessors are not generic.
- And that the canonical constructor, if user defined, is not 
explicitly invoking any other constructor.

- The list of forbidden record component names has also been updated.
- new error messages have been added

APIs:
- there have been some API editing in java.lang.Record, 
java.lang.runtime.ObjectMethods and java.lang.reflect.RecordComponent, 
java.io.ObjectInputStream, javax.lang.model (some visitors were added)


On the JVM implementation:
- some logging capabilities have been added to classFileParser.cpp to 
provide the reason for which the Record attribute has been ignored


Reflection:
- there are several new changes to the implementation of 
java.lang.reflect.RecordComponent apart from the spec changes 
mentioned before.


bug fixes in
- compiler
- serialization,
- JVM, etc

As a reference the last iteration of the previous reviews can be found 
at [4] under folders: compiler, hotspot_runtime, javadoc, reflection 
and serialization,


TIA,
Vicente

[1] 
http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.00/
[2] 
http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jls.html
[3] 
http://cr.openjdk.java.net/~gbierman/jep359/jep359-20191125/specs/records-jvms.html

[4] http://cr.openjdk.java.net/~vromero/records.review/

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[coleen . phillimore]

On 8/14/19 3:42 PM, Mandy Chung wrote:
I have further discussion with Coleen and walkthrough the vframe 
implementation.  Here is what we confirm and agree.


vframeStream::bci might return an invalid bci whereas javaVFrame::bci 
returns a valid bci (compiledVFrame::bci adjusts invalid bci to 0).


An invalid bci could happen when hitting a safepoint on bci #0 on a 
synchronized method either before or after the monitor is locked 
(implicit synchronization without explicit monitorenter).   That might 
occur when StackOverflowError is thrown for example. However, that 
should never happen when StackWalker is called and the top frame would 
be in the stack walking code.


+1/-1 adjustment intends to address invalid bci case but such case is 
not applicable for StackWalker API.  With that, I agree with Coleen 
that VM should set the bci value to StackFrameInfo::bci field and no 
adjustment needed:

   http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.05/


This looks good and straighforward.
Thank you!
Coleen



thanks
Mandy

On 8/13/19 4:56 PM, coleen.phillim...@oracle.com wrote:


Hi, I saw my name in this thread and had a discussion with Mandy. I 
don't like that the VM and JDK having this special coordinated dance 
of +1/-1, and the reason for this is to differentiate the value of 0 
in StackFrame meaning either uninitialized or invalid. If through 
some race, an unitialized value is observed, then the MemberName may 
not be filled in either.  In any case zero makes sense to return as 
the bci because it'll point to the line number beginning of the 
method, if used to get the line number.


The stackwalk API doesn't return invalid bci because it adjusts it:

int compiledVFrame::bci() const {
  int raw = raw_bci();
  return raw == SynchronizationEntryBCI ? 0 : raw;
}

At any rate, the 04 webrev looks good minus the +1/-1 dance and 
StackWalker.java comment.  Coupling the jdk and jvm like this feels 
like it's asking for bugs.  I like the simpler implementation that 
fixes the bug that we have.


Thanks,
Coleen


On 8/13/19 1:49 PM, Mandy Chung wrote:



On 8/13/19 9:30 AM, Peter Levart wrote:
Usually the StackFrameInfo(s) are consumed as soon as they are 
returned from StackWalker API and never assigned to @Stable field. 
So there's no purpose of @Stable for bci field use. Except 
documentation. But documentation can be specified in the form of 
comments too.




There are several JDK classes whose fields are filled by VM and 
comment is the form of documentation.  Until new use of StackFrame 
in the future shows performance benefit, it's okay to stick with 
@Stable original intent and keep the comment:


+    private int bci;    // zero and negative values 
represent invalid BCI


Please also review CSR:
   https://bugs.openjdk.java.net/browse/JDK-8229487

Mandy

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[coleen . phillimore]

Hi, I saw my name in this thread and had a discussion with Mandy. I 
don't like that the VM and JDK having this special coordinated dance of 
+1/-1, and the reason for this is to differentiate the value of 0 in 
StackFrame meaning either uninitialized or invalid.  If through some 
race, an unitialized value is observed, then the MemberName may not be 
filled in either.  In any case zero makes sense to return as the bci 
because it'll point to the line number beginning of the method, if used 
to get the line number.


The stackwalk API doesn't return invalid bci because it adjusts it:

int compiledVFrame::bci() const {
  int raw = raw_bci();
  return raw == SynchronizationEntryBCI ? 0 : raw;
}

At any rate, the 04 webrev looks good minus the +1/-1 dance and 
StackWalker.java comment.  Coupling the jdk and jvm like this feels like 
it's asking for bugs.  I like the simpler implementation that fixes the 
bug that we have.


Thanks,
Coleen


On 8/13/19 1:49 PM, Mandy Chung wrote:



On 8/13/19 9:30 AM, Peter Levart wrote:
Usually the StackFrameInfo(s) are consumed as soon as they are 
returned from StackWalker API and never assigned to @Stable field. So 
there's no purpose of @Stable for bci field use. Except 
documentation. But documentation can be specified in the form of 
comments too.




There are several JDK classes whose fields are filled by VM and 
comment is the form of documentation.  Until new use of StackFrame in 
the future shows performance benefit, it's okay to stick with @Stable 
original intent and keep the comment:


+    private int bci;    // zero and negative values 
represent invalid BCI


Please also review CSR:
   https://bugs.openjdk.java.net/browse/JDK-8229487

Mandy

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[coleen . phillimore]

On 5/7/19 9:36 AM, Lindenmaier, Goetz wrote:

Hi,

Please have a look at this further improved webrev:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09-incremental/

Harold, Coleen, thanks for pointing me to those methods.
I'm using them now. This simplifies the helper methods
considerably.

Ralf, thanks for looking at the messages!
I now suppress the "static " and
"The return value of '" strings in the array subscript
expressions and added corresponding test cases.

About "can not" versus "cannot", what I find in the
net "cannot" is to be preferred.  Any comments on that?
By native speakers?
See example messages here:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/09/output_with_debug_info.txt


Cannot is apparently preferable in English.  Native speaker (only 
language) but somebody had to tell me.


Coleen


Further, I fixed a build issue with the solaris compiler.
All handling of bci is now unsigned.

Best regards,
   Goetz.





-Original Message-
From: Schmelter, Ralf
Sent: Dienstag, 7. Mai 2019 14:35
To: Lindenmaier, Goetz ; Java Core Libs ; hotspot-runtime-...@openjdk.java.net; Coleen
Phillimore (coleen.phillim...@oracle.com) 
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
describing what is null.

Hi Goetz,

I've played with the messages a little bit and they generally look good. But 
I've
come across two which look strange:
  - 'o[static Test.INDEX]' is null. Can not invoke method 'int
java.lang.Object.hashCode()'
  - 'o[The return value of 'int java.lang.String.hashCode()]' is null. Can not 
invoke
method 'int java.lang.Object.hashCode()'.

Here is the test program to reproduce these:
public class Test {
 public static int INDEX = 2;

 public static void main(String[] args) {
 Object[] o = new Object[10];
 try {
  o[INDEX].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 try {
  o["".hashCode()].hashCode();
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
}

And 'Can not' should be 'Cannot'?

Best regards,
Ralf

Re: JEP 306

[coleen . phillimore]

This seems like better mailing list for this inquiry.
Coleen

On 5/9/18 6:43 AM, A Z wrote:

Does anyone know what is going to be decided
around full target status of JEP 306?

I have been recommended to this list for the sake
of this question.

?

http://openjdk.java.net/jeps/306
https://bugs.openjdk.java.net/browse/JDK-8175916

Re: RFR 8184289: Obsolete -XX:+UnsyncloadClass and -XX:+MustCallLoadClassInternal options

[coleen . phillimore]

On 2/12/18 1:54 AM, David Holmes wrote:

Hi Harold,

Adding core-libs-dev so they are aware of the ClassLoader change.

On 10/02/2018 5:44 AM, harold seigel wrote:

Hi David,

Thanks for reviewing this.

Please see updated webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8184289.2/webrev/index.html


This all seems fine to me.

I agree there is question mark over the SystemDictionary code now only 
used for the null/boot loader:


 848   if ((class_loader.is_null())) {
 849 if (k == NULL && HAS_PENDING_EXCEPTION
 850   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {

 851   MutexLocker mu(SystemDictionary_lock, THREAD);
 852   InstanceKlass* check = find_class(d_hash, name, 
dictionary);

 853   if (check != NULL) {
 854 // Klass is already loaded, so just use it
 855 k = check;
 856 CLEAR_PENDING_EXCEPTION;
 857 guarantee((!class_loader.is_null()), "dup definition 
for bootstrap loader?");

 858   }
 859 }

This seems to be a negative test, that we should in fact never get in 
this situation when dealing with the boot loader. But in that case we 
don't need lines 855 and 856 anymore and the code would just collapse to:


 848   if ((class_loader.is_null())) {
 849 if (k == NULL && HAS_PENDING_EXCEPTION
 850   && 
PENDING_EXCEPTION->is_a(SystemDictionary::LinkageError_klass())) {

 851   MutexLocker mu(SystemDictionary_lock, THREAD);
 852   guarantee(find_class(d_hash, name, dictionary) == NULL,
 853 "dup definition for bootstrap loader?");
 854 }
 855   }

and in that case I'd probably rather see this as an assertion than a 
guarantee, and the whole block can be debug-only.


(just to you two).  If this is decided that it's an assert only, my vote 
would be to remove it to simplify the logic here, which is the main 
benefit of removing these options.  It's too special-casey as it is.

Coleen


Thanks,
David
-


And, please see in-line comments.


On 2/8/2018 5:42 PM, David Holmes wrote:

Hi Harold,

First I'm pretty sure this one can't be pushed until the version 
bump arrives in jdk/hs :)

I hope the version bump arrives soon.


On 9/02/2018 6:53 AM, harold seigel wrote:

Hi,

Please review this JDK-11 change to obsolete the UnsyncloadClass 
and MustCallLoadClassInternal options.  With this change, these 
options are still accepted on the command line but have no affect 
other than to generate these warning messages:


    Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
    UnsyncloadClass; support was removed in 11.0

    Java HotSpot(TM) 64-Bit Server VM warning: Ignoring option
    MustCallLoadClassInternal; support was removed in 11.0

Open Webrev: 
http://cr.openjdk.java.net/~hseigel/bug_8184289/webrev/index.html


Overall looks good. Tricky to untangle things in the SD especially!

src/hotspot/share/classfile/systemDictionary.cpp

Looking at this change, and the comment:

-   // For UnsyncloadClass only
    // If they got a linkageError, check if a parallel class 
load succeeded.
    // If it did, then for bytecode resolution the specification 
requires
    // that we return the same result we did for the other 
thread, i.e. the

    // successfully loaded InstanceKlass
    // Should not get here for classloaders that support 
parallelism
    // with the new cleaner mechanism, even with 
AllowParallelDefineClass
    // Bootstrap goes through here to allow for an extra 
guarantee check

!   if (UnsyncloadClass || (class_loader.is_null())) {

It's not clear why all the "UnsyncLoadClass only" stuff is also 
being done for the "Bootstrap" (? bootloader?) case. But in any case 
the comment block doesn't read correctly now as this is all, and 
only, about the bootstrap case. I'd suggest:


-   // For UnsyncloadClass only
+   // For bootloader only:
    // If they got a linkageError, check if a parallel class 
load succeeded.
    // If it did, then for bytecode resolution the specification 
requires
    // that we return the same result we did for the other 
thread, i.e. the

    // successfully loaded InstanceKlass
    // Should not get here for classloaders that support 
parallelism
    // with the new cleaner mechanism, even with 
AllowParallelDefineClass
-   // Bootstrap goes through here to allow for an extra 
guarantee check

Done.



Question: is ClassLoader.loadClassInternal now obsolete as well?
Yes.  Thanks for pointing that out.  The new webrev contains 
significant changes needed to remove loadClassInternal.


---

src/hotspot/share/runtime/arguments.cpp

Is there some reason to leave the expiration version unset? Do you 
think the obsoletion warning may be needed for a couple of releases ??

I figured whoever expires the option can put in the version.


---

 test/hotspot/jtreg/runtime/CommandLine/ObsoleteFla

Re: RFR (S) 8173715: Remove FlatProfiler

[coleen . phillimore]

Besides the launcher.properties files of other languages, there was a 
bsd file in the jdk that had -Xprof:


bsd/doc/man/java.1:\-Xprof

Does that have to be updated.  Otherwise I think it looks good.

thanks,
Coleen


On 8/16/17 1:48 PM, Gerard Ziemski wrote:

hi all,

(this is jdk part of the change, the hotspot part is being reviewed on 
hotspot-...@openjdk.java.net)

The FlatProfiler (i.e. -Xprof) has been deprecated for a while, and now it’s 
the time to remove it.

Note about java's man page international translations: I was told by the 
localization team, that only changes to the English man pages are needed, and 
that they will trigger an update to other translations.

issue:  https://bugs.openjdk.java.net/browse/JDK-8173715
webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev1_jdk

passes the following tests:

  JPRT
  RBT test_hotspot/test/serviceability/jvmti
  RBT test_hotspot/test/gc
  RBT test_hotspot/test/compiler
  RBT test_hotspot/test/runtime

Thank you.

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

[Coleen Phillimore]

Thanks Brent, looks good!
Coleen

On 1/31/17 1:19 PM, Brent Christian wrote:

On 1/30/17 5:52 PM, Coleen Phillimore wrote:


in
http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp.html 



 287 int mode = 0;
 288 if (_jvf->is_interpreted_frame()) { mode |= 
MODE_INTERPRETED; }
 289 if (_jvf->is_compiled_frame()){ mode |= 
MODE_COMPILED;}


The mode can't be interpreted AND compiled so the OR doesn't make
sense here.  There may be other options though, so I wouldn't make
these the only cases.   It should be something more like:

  if (_jvf->is_interpreted_frame()) {
mode = MODE_INTERPRETED;
  else if (_jvf->is_compiled_frame()) {
mode = MODE_COMPILED;
  }

with no 'else' because it could be entry or runtime stub or new things
that we may eventually add, that you should probably not tell the API.


Thanks - makes sense.  Webrev updated in place.


Otherwise this seems fine.   I didn't see the update to test "On the
hotspot side, I had to update the 8163014 regtest. "


Ah, that's this:

http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/test/runtime/LocalLong/LocalLongHelper.java.sdiff.html 



though the main @test file, .../LocalLong/LocalLongTest.java, didn't 
need any changes.



Please make sure JPRT -testset hotspot works with this (or check it in
that way).


Yes, that runs cleanly, and I do plan to push through hs.

Thanks for having a look, Coleen.

-Brent


On 1/26/17 8:07 PM, Brent Christian wrote:

Hi,

Please review my updated approach for fixing 8156073 ("2-slot
LiveStackFrame locals (long and double) are incorrect") in the
experimental portion of the StackWalker API, added in JDK 9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/

(For reference, the earlier RFR thread is here:[1].)

We've concluded that we can't get enough primitive type info from the
VM to use the current fine-grain *Primitive classes in
LiveStackFrameInfo, so those classes have been removed for now. I've
simplified down to just a PrimitiveSlot32 for 32-bit VMs, and
PrimitiveSlot64 for 64-bit VMs.

We also do not attempt to interpret a primitive's type based on the
slot's contents, and instead return the slot's full contents for
every primitive local.  This more accurately reflects the underlying
layout of locals in the VM (versus the previous proposed
LiveStackFrame.getLocals() behavior of having 64-bit behave like
32-bit by splitting long/double values into two slots).  On the
64-bit VM, this returns 64-bits even for primitive local variables
smaller than 64-bits (int, etc).

The upshot is that we now return the entire value for longs/doubles
on 64-bit VMs.  (The current JDK 9 only reads the first 32-bits.)

I also now indicate in LiveStackFrameInfo.toString() if the frame is
interpreted or compiled.

On the testing front:
In the interest of simplifying LiveStackFrame testing down into a
single test file under jdk/test/, I chose not to add the additional
test case from Fabio Tudone [2,3] (thanks, Fabio!), but instead
incorporated some of those testing ideas into the existing test.
Testing is a bit relaxed versus the previously proposed test case; in
particular it does not enforce endianness.

I also removed the more simplistic CountLocalSlots.java test, given
that the updated LocalsAndOperands.java will now better cover testing
-Xcomp.  On the hotspot side, I had to update the 8163014 regtest.

Automated test runs have looked fine so far.

Thanks,
-Brent

1.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042979.html 



2.
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041666.html 



3. https://bugs.openjdk.java.net/browse/JDK-8158879

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

[Coleen Phillimore]

Added core-libs-dev.
Coleen

On 1/30/17 5:52 PM, Coleen Phillimore wrote:


Hi Brent,

I think this looks more conservative and better.

in
http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/hotspot/src/share/vm/prims/stackwalk.cpp.html 



 287 int mode = 0;
 288 if (_jvf->is_interpreted_frame()) { mode |= MODE_INTERPRETED; }
 289 if (_jvf->is_compiled_frame()){ mode |= MODE_COMPILED;}

The mode can't be interpreted AND compiled so the OR doesn't make 
sense here.  There may be other options though, so I wouldn't make 
these the only cases.   It should be something more like:


  if (_jvf->is_interpreted_frame()) {
mode = MODE_INTERPRETED;
  else if (_jvf->is_compiled_frame()) {
mode = MODE_COMPILED;
  }

with no 'else' because it could be entry or runtime stub or new things 
that we may eventually add, that you should probably not tell the API.


Otherwise this seems fine.   I didn't see the update to test "On the 
hotspot side, I had to update the 8163014 regtest. "


Please make sure JPRT -testset hotspot works with this (or check it in 
that way).


Thanks,
Coleen

On 1/26/17 8:07 PM, Brent Christian wrote:

Hi,

Please review my updated approach for fixing 8156073 ("2-slot
LiveStackFrame locals (long and double) are incorrect") in the 
experimental portion of the StackWalker API, added in JDK 9.


Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/

(For reference, the earlier RFR thread is here:[1].)

We've concluded that we can't get enough primitive type info from the 
VM to use the current fine-grain *Primitive classes in 
LiveStackFrameInfo, so those classes have been removed for now. I've 
simplified down to just a PrimitiveSlot32 for 32-bit VMs, and 
PrimitiveSlot64 for 64-bit VMs.


We also do not attempt to interpret a primitive's type based on the 
slot's contents, and instead return the slot's full contents for 
every primitive local.  This more accurately reflects the underlying 
layout of locals in the VM (versus the previous proposed 
LiveStackFrame.getLocals() behavior of having 64-bit behave like 
32-bit by splitting long/double values into two slots).  On the 
64-bit VM, this returns 64-bits even for primitive local variables 
smaller than 64-bits (int, etc).


The upshot is that we now return the entire value for longs/doubles 
on 64-bit VMs.  (The current JDK 9 only reads the first 32-bits.)


I also now indicate in LiveStackFrameInfo.toString() if the frame is 
interpreted or compiled.


On the testing front:
In the interest of simplifying LiveStackFrame testing down into a 
single test file under jdk/test/, I chose not to add the additional 
test case from Fabio Tudone [2,3] (thanks, Fabio!), but instead 
incorporated some of those testing ideas into the existing test.  
Testing is a bit relaxed versus the previously proposed test case; in 
particular it does not enforce endianness.


I also removed the more simplistic CountLocalSlots.java test, given 
that the updated LocalsAndOperands.java will now better cover testing 
-Xcomp.  On the hotspot side, I had to update the 8163014 regtest.


Automated test runs have looked fine so far.

Thanks,
-Brent

1. 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042979.html
2. 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041666.html

3. https://bugs.openjdk.java.net/browse/JDK-8158879

Re: RFR: 8163014: Mysterious/wrong value for "long" frame local variable on 64-bit

[Coleen Phillimore]

On 9/7/16 9:09 AM, Max Ockner wrote:
Does the stackwalk API have access to the type of variable at each 
slot? Where is this information stored? My operating assumption was 
that it did not have this information, and therefore needed to read 
the garbage slot.


This is true.  The StackWalk API does not have type information for 
these longs, so this prevents garbage values from being read.

Adding core-libs.

Thanks,
Coleen


Max

On 9/6/2016 8:24 PM, dean.l...@oracle.com wrote:
Instead of fixing this at write time, how about instead fixing it at 
read time?  That was my understanding of John's comment:


A virtualized API which
produces a view on such an L[1] needs to return some
default value (if pressed), and to indicate that the slot
has no payload.

SoLiveStackFrame.getLocals() would never read the physical garbage 
slot, but instead would act as if it
read 0 instead.  And by LiveStackFrame.getLocals() I really mean 
whatever code fills in its "locals" field,

which I guess is the JVM.

dl

On 9/6/16 2:33 PM, Max Ockner wrote:
Hello, Please review this multi-platform fix for the stack walk API. 
Bug: https://bugs.openjdk.java.net/browse/JDK-8163014 Webrev: 
http://cr.openjdk.java.net/~mockner/8163014.01/ In 64 bits, long 
values can fit into a single slot, but two slots are still used. The 
high slot contains garbage. This normally wouldn't matter since it 
is never read from but the stack walk API expects to display useful 
information. This is an issue when displaying longs from local 
variables, so this means we can kill any garbage off by zeroing it 
when it is pushed to the stack in the previous frame. This solution 
zeroes the high byte of a long value when it is being pushed to the 
stack (in push_l). This applies to x86, aarch64, and sparc. This 
change does not apply to ppc, though the bug almost certainly does 
affect it. I have left ppc untouched since I don't have access to 
the hardware required to reproduce the bug and test the fix. I have 
adapted the reproducer from the bug into a test. It is curently 
sitting in runtime/locallong, but I believe there must be a better 
place for it. Thanks, Max 

Re: JDK 9 RFR of JDK-8162539: Test fails because it expects a blank between method signature and throws exception

[Coleen Phillimore]

On 7/26/16 3:36 PM, joe darcy wrote:


Hi Coleen,

The existing tests covered *toGenericString* output with a throws 
clauses but omitted coverage of *toString* methods with a throws 
clause. That let the omission of the space character in toString 
output slip through.


(There is some logically duplicated structure in the implementations 
of the toString and toGenericString methods.)


On the core libs side, I believe there weren't any regression tests of 
the toString output  of core reflection objects until the 
toGenericString methods were added in JDK 5. That is one reason the 
test are weighted toward toGenericString since the tests were added 
for that functionality.


HTH,



Yes, thanks.  Glad there's a test now.
Coleen


-Joe


On 7/26/2016 12:23 PM, Coleen Phillimore wrote:
Thank you for fixing this so quickly.  This looks good but I have a 
question about:


http://cr.openjdk.java.net/~darcy/8162539.0/test/java/lang/reflect/Constructor/GenericStringTest.java.udiff.html
  @ExpectedGenericString(
 "protected  TestClass1(S,T) throws java.lang.Exception")
+ @ExpectedString(
+ "protected TestClass1(java.lang.Object,java.lang.Object) throws 
java.lang.Exception")

  protected  TestClass1(S s, T t) throws Exception{}

I can't really read the metaprogramming but why didn't the existing 
@Expected{Generic}String strings here find the problem?


thanks,
Coleen

On 7/26/16 3:08 PM, joe darcy wrote:

Hello,

Please review the changes to address

JDK-8162539: Test fails because it expects a blank between 
method signature and throws exception

http://cr.openjdk.java.net/~darcy/8162539.0/

In brief, recent refactorings of the toString output in core 
reflection (JDK-8161500 Use getTypeName and StringJoiner in core 
reflection generic toString methods) omitted a space character 
between the closing ")" and "throws" for toString output, but 
correctly included the space in toGenericString output.


The simple fix is to add the space character; regression tests are 
suitably augmented and slightly refactored.


Thanks,

-Joe

Re: JDK 9 RFR of JDK-8162539: Test fails because it expects a blank between method signature and throws exception

[Coleen Phillimore]

Thank you for fixing this so quickly.  This looks good but I have a 
question about:


http://cr.openjdk.java.net/~darcy/8162539.0/test/java/lang/reflect/Constructor/GenericStringTest.java.udiff.html

 @ExpectedGenericString(
"protected  TestClass1(S,T) throws java.lang.Exception")
+ @ExpectedString(
+ "protected TestClass1(java.lang.Object,java.lang.Object) throws 
java.lang.Exception")

 protected  TestClass1(S s, T t) throws Exception{}


I can't really read the metaprogramming but why didn't the existing 
@Expected{Generic}String strings here find the problem?


thanks,
Coleen

On 7/26/16 3:08 PM, joe darcy wrote:

Hello,

Please review the changes to address

JDK-8162539: Test fails because it expects a blank between method 
signature and throws exception

http://cr.openjdk.java.net/~darcy/8162539.0/

In brief, recent refactorings of the toString output in core 
reflection (JDK-8161500 Use getTypeName and StringJoiner in core 
reflection generic toString methods) omitted a space character between 
the closing ")" and "throws" for toString output, but correctly 
included the space in toGenericString output.


The simple fix is to add the space character; regression tests are 
suitably augmented and slightly refactored.


Thanks,

-Joe

Re: Advice on cross-repo change: JDK-8160997

[Coleen Phillimore]

Hi, You can send this to core-libs-dev@openjdk.java.net and 
hotspot-...@openjdk.java.net


The sponsor should be from the runtime group.   I don't know this well 
enough, so I'm going to point you to Dan (and Jerry, except Jerry's not 
a committer).


To run JPRT don't forget to use -testset hotspot and you'll get zero 
failures.


Coleen


On 7/7/16 3:30 PM, Alan Burlison wrote:

Hi,

I've peeled another one off my wad of patches, this one gits rid of 
the use of deprecated  and  APIs and switches to the 
POSIX ones. This works on both S11 and S12 - without it the JDK 
doesn't build on S12.


The change crosses repos so I'm looking for some advice on where best 
to submit it for review?


Files modified:
hotspot/src/os/solaris/vm/os_solaris.cpp
hotspot/src/os/solaris/vm/perfMemory_solaris.cpp
hotspot/src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
hotspot/src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
jdk/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c
jdk/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c
jdk/src/jdk.security.auth/solaris/native/libjaas/Solaris.c
jdk/src/jdk.security.auth/unix/native/libjaas/Unix.c

Bug:
https://bugs.openjdk.java.net/browse/JDK-8160997

Webrev:
http://jurassic.us.oracle.com/~alanbur/jdk/JDK-8160997/

Webrev zip:
http://jurassic.us.oracle.com/~alanbur/jdk/JDK-8160997.zip

Patch (also attached to bug):
http://jurassic.us.oracle.com/~alanbur/jdk/JDK-8160997.patch

JPRT results - 1 failure, looks unrelated
http://sthjprt.uk.oracle.com/archives/2016/07/2016-07-07-172040.alanbur.getpw/ 



Thanks,

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

On 4/5/16 7:48 PM, Brent Christian wrote:
Thanks, Coleen.  Coordinating method/function names on "to stack trace 
element" is a fine thing.  I've done so in the updated webrev, and 
also implemented Claes's suggestion.


http://cr.openjdk.java.net/~bchristi/8153123/webrev.01/index.html


Thank you for making this change.  It looks good!  I've reviewed this.

Coleen



-Brent
On 04/05/2016 11:25 AM, Coleen Phillimore wrote:


A correction below.

On 4/5/16 1:29 PM, Coleen Phillimore wrote:


Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes.
This looks really good.

One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to
Throwable::fill_in_stack_trace().

-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject
frame, jobject stack))
   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info,
stack_trace_element, THREAD); JVM_END


And the function is called fill_methodInfo in the javaClasses 
function.


I think the JVM and the java_lang_StackFrameInfo function names
should be closer.

I wonder if the name JVM_ToStackFrameElement() and
java_lang_StackFrameInfo::to_stack_frame_element() would be better
and then it'd match the Java name.



I meant JVM_ToStackTraceElement() and
java_lang_StackFrameInfo::to_stack_trace_element(), since it's producing
a StackTraceElement.

thanks,
Coleen

Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:

On Apr 4, 2016, at 4:45 PM, Brent Christian
 wrote:

Hi,

I'd like to check in some footprint and code reduction changes to
the java.lang.StackWalker implementation.

Webrev:
http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8153123


This looks good to me.

One thing to mention is that this patch is a follow-up work from the
investigation on what it takes to enable Throwable to use
StackWalker (JDK-8141239). The current built-in VM backtrace is very
compact and performant.  We have identified and prototypes the
performance improvements if Throwable backtrace is generated using
stack walker.  There are some performance gaps that we agree to
defer JDK-8141239 to a future release and improve the footprint
performance and GC throughput concerns when MemberNames are stored
in the throwable backtrace.

Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

A correction below.

On 4/5/16 1:29 PM, Coleen Phillimore wrote:


Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes. 
This looks really good.


One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to 
Throwable::fill_in_stack_trace().


-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject 
frame, jobject stack))

   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info, 
stack_trace_element, THREAD); JVM_END



And the function is called fill_methodInfo in the javaClasses function.

I think the JVM and the java_lang_StackFrameInfo function names 
should be closer.


I wonder if the name JVM_ToStackFrameElement() and 
java_lang_StackFrameInfo::to_stack_frame_element() would be better 
and then it'd match the Java name.




I meant JVM_ToStackTraceElement() and 
java_lang_StackFrameInfo::to_stack_trace_element(), since it's producing 
a StackTraceElement.


thanks,
Coleen

Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:
On Apr 4, 2016, at 4:45 PM, Brent Christian 
 wrote:


Hi,

I'd like to check in some footprint and code reduction changes to 
the java.lang.StackWalker implementation.


Webrev:
http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8153123


This looks good to me.

One thing to mention is that this patch is a follow-up work from the 
investigation on what it takes to enable Throwable to use 
StackWalker (JDK-8141239). The current built-in VM backtrace is very 
compact and performant.  We have identified and prototypes the 
performance improvements if Throwable backtrace is generated using 
stack walker.  There are some performance gaps that we agree to 
defer JDK-8141239 to a future release and improve the footprint 
performance and GC throughput concerns when MemberNames are stored 
in the throwable backtrace.


Mandy

Re: RFR 8153123 : Streamline StackWalker code

[Coleen Phillimore]

Also meant to include core-libs-dev in the email.
Thanks,
Coleen

On 4/5/16 1:27 PM, Coleen Phillimore wrote:


Hi, I've reviewed the hotspot changes and some of the jdk changes. 
This looks really good.


One comment about the jvm function names:

I think FillInStackTraceElement is too close of a name to 
Throwable::fill_in_stack_trace().


-JVM_ENTRY(void, JVM_SetMethodInfo(JNIEnv *env, jobject frame))
+JVM_ENTRY(void, JVM_FillInStackTraceElement(JNIEnv *env, jobject 
frame, jobject stack))

   JVMWrapper("JVM_SetMethodInfo");
- Handle stackFrame(THREAD, JNIHandles::resolve(frame));
- java_lang_StackFrameInfo::fill_methodInfo(stackFrame, THREAD);
+ Handle stack_frame_info(THREAD, JNIHandles::resolve(frame));
+ Handle stack_trace_element(THREAD, JNIHandles::resolve(stack));
+ java_lang_StackFrameInfo::fill_methodInfo(stack_frame_info, 
stack_trace_element, THREAD); JVM_END



And the function is called fill_methodInfo in the javaClasses function.

I think the JVM and the java_lang_StackFrameInfo function names should 
be closer.


I wonder if the name JVM_ToStackFrameElement() and 
java_lang_StackFrameInfo::to_stack_frame_element() would be better and 
then it'd match the Java name.


Thanks!
Coleen

On 4/4/16 9:29 PM, Mandy Chung wrote:
On Apr 4, 2016, at 4:45 PM, Brent Christian 
 wrote:


Hi,

I'd like to check in some footprint and code reduction changes to 
the java.lang.StackWalker implementation.


Webrev:
http://cr.openjdk.java.net/~bchristi/8153123/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8153123


This looks good to me.

One thing to mention is that this patch is a follow-up work from the 
investigation on what it takes to enable Throwable to use StackWalker 
(JDK-8141239). The current built-in VM backtrace is very compact and 
performant.  We have identified and prototypes the performance 
improvements if Throwable backtrace is generated using stack walker.  
There are some performance gaps that we agree to defer JDK-8141239 to 
a future release and improve the footprint performance and GC 
throughput concerns when MemberNames are stored in the throwable 
backtrace.


Mandy

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

On 3/11/16 6:36 PM, Aleksey Shipilev wrote:

On 03/08/2016 01:55 AM, Coleen Phillimore wrote:

Aside: see the last experiment, avoiding StringTable::intern (shows in
profiles a lot!) trims down construction costs down even further. I'd
think that is a worthwhile improvement to consider.

Hm, this is an interesting experiment.  I've been looking for a better
way to store the name of the method rather than cpref.

I did some preliminary work for storing class names (those are easy,
since Class.name is already there on Java side). Would be nice to handle
both method names and source files, because we are looking at some nice
improvements:
   https://bugs.openjdk.java.net/browse/JDK-8151751

Can you pick it up, and follow up further?


Yes, I think caching String classname on Class might be also helpful 
for the StackWalk API.


My impression was that the performance of Throwable.getStackTrace() 
wasn't super critical since it's used in exceptional conditions. Let me 
know otherwise.


I linked the bug I care about to this.

thanks,
Coleen



Cheers,
-Aleksey

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

Hi Aleksey,  This is an interesting experiment.

On 3/4/16 8:28 AM, Aleksey Shipilev wrote:

On 03/02/2016 11:21 PM, Aleksey Shipilev wrote:

On 03/02/2016 10:57 PM, Coleen Phillimore wrote:

On 3/2/16 1:58 PM, Aleksey Shipilev wrote:

Is there an underlying reason why we can't return the pre-filled
StackTraceElements[] array from the JVM_GetStackTraceElements to begin
with? This will avoid leaking StackTraceElement constructor into
standard library, *and* allows to make StackTraceElement fields final.
Taking stuff back from the standard library is hard, if not impossible,
so we better expose as little as possible.

We measured that it's faster to allocate the StackTraceElement array
in Java and it seems cleaner to the Java guys. It came from similar
code we've been prototyping for StackFrameInfo.

OK, it's not perfectly clean from implementation standpoint, but this
RFE might not be the best opportunity to polish that. At least make
StackTraceElement constructor private (better), or package-private
(acceptable), and then we are good to go.

Okay, here's a little exploration:
   http://cr.openjdk.java.net/~shade/8150778/StackTraceBench.java

The difference between allocating in Java code, and allocating on VM
side is marginal on my machine, but I think we are down to native memset
performance when allocating on VM side. Therefore, I'd probably stay
with Java allocation which codegen we absolutely control.


Thanks for the experiment.  We measured a greater performance 
difference.  The theory is that through Java, allocation is a TLAB 
pointer update in most cases, vs going through all the C++ code to do 
allocation.  The small difference for performance here isn't critical, 
but having the allocation in Java looks nicer to the Java programmers here.

Aside: see the last experiment, avoiding StringTable::intern (shows in
profiles a lot!) trims down construction costs down even further. I'd
think that is a worthwhile improvement to consider.


Hm, this is an interesting experiment.  I've been looking for a better 
way to store the name of the method rather than cpref.


thanks,
Coleen


Cheers,
-Aleksey

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

Mandy, thank you for reviewing this.

On 3/2/16 9:18 PM, Mandy Chung wrote:

On Mar 2, 2016, at 4:03 PM, Coleen Phillimore  
wrote:

Freshly tested changes with jck tests, with missing checks and other changes to 
use the depth field, as pointed out by Aleksey.  I've kept the 
StackTraceElement[] allocation in Java to match the new API that was approved.

open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_hotspot/
open webrev at http://cr.openjdk.java.net/~coleenp/8150778.02_jdk/

typo in your link:
   http://cr.openjdk.java.net/~coleenp/8150778.02_jck/

StackTraceElement.java
  80  * @since 1.9


Okay, good because it's probably 9.0 anyway.


This is not needed. Simply take this out.

Throwable.java

215  * Native code sets the depth of the backtrace for later retrieval

s/Native code/VM/
I changed it to "The JVM sets the depth..."  There was another sentence 
just like this for the backtrace field, which I also changed.

since VM is setting the depth field.


896 private native void getStackTraceElements(StackTraceElement[] elements);

Can you add the method description
“Gets the stack trace elements."


Fixed.

I only skimmed on the hotspot change.  Looks okay to me.

TestThrowable.java

   43 int getDepth(Throwable t) throws Exception {
   44   for (Field f : Throwable.class.getDeclaredFields()) {
   45 if (f.getName().equals("depth")) {
  


You can replace the above with Throwable.class.getDeclaredField(“depth”)


Yes, that's better.

Otherwise, looks okay.


Thanks!
Coleen

Mandy

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

On 3/2/16 3:21 PM, Aleksey Shipilev wrote:

On 03/02/2016 10:57 PM, Coleen Phillimore wrote:

On 3/2/16 1:58 PM, Aleksey Shipilev wrote:

Is there an underlying reason why we can't return the pre-filled
StackTraceElements[] array from the JVM_GetStackTraceElements to begin
with? This will avoid leaking StackTraceElement constructor into
standard library, *and* allows to make StackTraceElement fields final.
Taking stuff back from the standard library is hard, if not impossible,
so we better expose as little as possible.

We measured that it's faster to allocate the StackTraceElement array
in Java and it seems cleaner to the Java guys. It came from similar
code we've been prototyping for StackFrameInfo.

OK, it's not perfectly clean from implementation standpoint, but this
RFE might not be the best opportunity to polish that. At least make
StackTraceElement constructor private (better), or package-private
(acceptable), and then we are good to go.
Well, the RFE is intended to clean this up but I don't think there's 
agreement about what the cleanest thing is.  If the cleaner API is:


   StackTraceElement[] getStackTraceElements();

we should change it once and not twice.  I'd like to hear other opinions!

Since StackTraceElement constructor is called by Throwable it has to be 
package private but can't be private.  I have made it package private.


Also, I think you can drop this line:
   836  int depth = getStackTraceDepth();


Oh, right, I can do that.  I was hiding the field depth.  i don't need 
the function either.


Thanks! Thank you for looking at this so quickly.

Coleen



Thanks,
-Aleksey

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

On 3/2/16 1:58 PM, Aleksey Shipilev wrote:

Hi Coleen,

On 03/02/2016 09:44 PM, Coleen Phillimore wrote:

Summary: replace JVM_GetStackTraceDepth and JVM_GetStackTraceElement,
with JVM_GetStackTraceElements that gets all the elements in the
StackTraceElement[]

These improvements were found during the investigation for replacing
Throwable with the StackWalkAPI.   This change also adds iterator for
BacktraceBuilder to make changing format of backtrace easier.

Tested with -testset core, RBT nightly hotspot nightly tests on all
platforms, and jck tests on linux x64.  Compatibility request is approved.

open webrev at http://cr.openjdk.java.net/~coleenp/8150778_jdk/
open webrev at http://cr.openjdk.java.net/~coleenp/8150778_hotspot
bug link https://bugs.openjdk.java.net/browse/JDK-8150778

Looks interesting!

Is there an underlying reason why we can't return the pre-filled
StackTraceElements[] array from the JVM_GetStackTraceElements to begin
with? This will avoid leaking StackTraceElement constructor into
standard library, *and* allows to make StackTraceElement fields final.
Taking stuff back from the standard library is hard, if not impossible,
so we better expose as little as possible.


We measured that it's faster to allocate the StackTraceElement array in 
Java and it seems cleaner to the Java guys.

It came from similar code we've been prototyping for StackFrameInfo.


Other minor nits:

  * Initializing fields to their default values is a code smell in Java:
  private transient int depth = 0;


ok, not sure what "code smell" means but it doesn't have to be 
initialized like this.  It's set in the native code.


  * Passing a null array to getStackTraceElement probably SEGVs? I don't
see the null checks in native parts.


Yes, it would SEGV.  I'll add some checks for null and make sure it's an 
array of StackTraceElement.


Thanks,
Coleen


Thanks,
-Aleksey

Re: RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

Hi Daniel,
Thank you for looking at this so quickly.

On 3/2/16 1:57 PM, Daniel Fuchs wrote:

Hi Coleen,

Nice improvement!

Two remarks on http://cr.openjdk.java.net/~coleenp/8150778_jdk/

1. StackTraceElement.java

Does the new constructor in StackTraceElement really need to be
public? Can't we keep that package protected?


So I just removed the public keyword, and that seems good.  Thanks!



2. Throwable.java:902

902  * package-protection for use by SharedSecrets.

If I'm not mistaken we removed the shared secrets access - IIRC that
was used by java.util.logging.LogRecord  - which now uses the
StackWalker API instead.

So maybe you could make the method private and remove the comment
as further cleanup.


I had just copied the SharedSecrets comments.  I'll make 
getStackTraceElements private also.




Please don't count me as (R)eviewer for the hotspot changes :-)


Oh, but you know this code in hotspot, now.  That's ok, you don't need 
to review hotspot code.


Thanks!
Coleen



best regards,

-- daniel

On 02/03/16 19:44, Coleen Phillimore wrote:

Summary: replace JVM_GetStackTraceDepth and JVM_GetStackTraceElement,
with JVM_GetStackTraceElements that gets all the elements in the
StackTraceElement[]

These improvements were found during the investigation for replacing
Throwable with the StackWalkAPI.   This change also adds iterator for
BacktraceBuilder to make changing format of backtrace easier.

Tested with -testset core, RBT nightly hotspot nightly tests on all
platforms, and jck tests on linux x64.  Compatibility request is 
approved.


open webrev at http://cr.openjdk.java.net/~coleenp/8150778_jdk/
open webrev at http://cr.openjdk.java.net/~coleenp/8150778_hotspot
bug link https://bugs.openjdk.java.net/browse/JDK-8150778

Thanks,
Coleen

RFR 8150778: Reduce Throwable.getStackTrace() calls to the JVM

[Coleen Phillimore]

Summary: replace JVM_GetStackTraceDepth and JVM_GetStackTraceElement, 
with JVM_GetStackTraceElements that gets all the elements in the 
StackTraceElement[]


These improvements were found during the investigation for replacing 
Throwable with the StackWalkAPI.   This change also adds iterator for 
BacktraceBuilder to make changing format of backtrace easier.


Tested with -testset core, RBT nightly hotspot nightly tests on all 
platforms, and jck tests on linux x64.  Compatibility request is approved.


open webrev at http://cr.openjdk.java.net/~coleenp/8150778_jdk/
open webrev at http://cr.openjdk.java.net/~coleenp/8150778_hotspot
bug link https://bugs.openjdk.java.net/browse/JDK-8150778

Thanks,
Coleen

Re: ClassFileTransformer does not apply to anonymous classes

[Coleen Phillimore]

On 1/22/16 4:11 AM, Andrew Dinn wrote:

On 21/01/16 22:14, Rafael Winterhalter wrote:

Hi Andrew,
if there is any update on the matter, I would of course love to hear about it.
Unfortunately, I never received an answer to my question, but maybe I
picked the wrong list.
Thank you for your support on this issue!

I'm pretty sure this is the right list. But I don't know who cold answer
the question. I know that Coleen Phillmore (added explicitly to CC)
often deals with ClassFileTransformer issues. Coleen, can you answer?


Can you send the question again?  I didn't see it.  I also mostly look 
at JVM code and rarely deal with the Java side.


I'm also adding Dan, Serguei and Markus.

Thanks,
Coleen


regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in UK and Wales under Company Registration No. 3798903
Directors: Michael Cunningham (US), Michael O'Neill (Ireland), Paul
Argiry (US)

Re: RFR [9] 8141615: Add new public methods to sun.reflect.ConstantPool

[Coleen Phillimore]

This seems fine.  Only one minor comment (where I don't need to see 
another webrev)


+ constantPoolHandle cp = constantPoolHandle(THREAD, 
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));


would be shorter as

+ constantPoolHandle cp(THREAD, 
sun_reflect_ConstantPool::get_cp(JNIHandles::resolve_non_null(obj)));


and avoid implicit copy construction.

Thanks for adding tests.

Coleen

On 1/11/16 8:34 AM, Konstantin Shefov wrote:

Kindly reminder.

Already approved by C. Thalinger and I. Ignatyev.

Thanks
-Konstantin

On 12/17/2015 11:26 AM, Konstantin Shefov wrote:

Hi Coleen

You have previously reviewed this enhancement and made a few comments
I have resolved them, so could you look at the webrevs again, please?

I have added tests, removed cp tags that are not in JVM spec (100 - 
105) and made some other changes.


JDK: http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04
HOTSPOT: http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02

Thanks
-Konstantin


On 12/16/2015 07:42 PM, Christian Thalinger wrote:

Looks good.  Thanks.

On Dec 16, 2015, at 1:13 AM, Konstantin Shefov 
 wrote:


Christian

I have fixed the enum so it uses "ENUMENTRY(int)" format now and 
does linear search.

http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.04/

-Konstantin

On 12/15/2015 08:36 PM, Christian Thalinger wrote:
On Dec 14, 2015, at 11:11 PM, Konstantin Shefov 
<mailto:konstantin.she...@oracle.com>> wrote:


Hi Christian

Thanks for reviewing, I have changed indents as you asked:

http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.03 
<http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.03>
Thanks.  I’m still not comfortable with the enum.  It would be 
great if we could get the values from the VM like in JVMCI:


http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/HotSpotConstantPool.java#l101 



but that would be overkill here.  But I would like to see the enum 
entries take the integer values as arguments, like here:


http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/c036c7f17e09/src/jdk.vm.ci/share/classes/jdk.vm.ci.sparc/src/jdk/vm/ci/sparc/SPARCKind.java#l27 



and either do a simple linear search to find the entry or build up 
a table like the HotSpotConstantPool example above.



-Konstantin

On 12/15/2015 06:23 AM, Christian Thalinger wrote:
On Dec 11, 2015, at 1:54 AM, Konstantin Shefov 
<mailto:konstantin.she...@oracle.com>> wrote:


Hello

Please review the new version on the patch.

New webrev:
Webrev hotspot: 
http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.02 
<http://cr.openjdk.java.net/%7Ekshefov/8141615/hotspot/webrev.02>
Webrev jdk: 
http://cr.openjdk.java.net/~kshefov/8141615/jdk/webrev.02 
<http://cr.openjdk.java.net/%7Ekshefov/8141615/jdk/webrev.02>
These newlines look ridiculous especially when it’s not even 
needed:


+  // Returns a class reference index for a method or a field.
+  public int  getClassRefIndexAt
+ (int index) { return 
getClassRefIndexAt0 (constantPoolOop, index); }


Either fix the indent or just add them like regular methods 
should look like.



What has been changed:
1. Added tests for the new added methods.
2. Removed CP tag codes 100 - 105 from being passed to java and 
left only codes from the open JVM spec 
(https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-4.html#jvms-4.4-140).


Thanks
-Konstantin

On 11/27/2015 07:48 PM, Konstantin Shefov wrote:

Coleen,
Thanks for review

On 11/24/2015 07:33 PM, Coleen Phillimore wrote:
I have a couple preliminary comments.  First, there are no 
tests added with all this new functionality.  Tests should be 
added with the functionality changeset, not promised afterward.

I will add tests.
Secondly, I do not like that JDK code knows the 
implementation details of the JVM's constant pool tags.  
These should be only for internal use.
The package "sun.reflect" is for internal use only, so it 
shouldn’t be a problem.
My third comment is that I haven't looked carefully at this 
constant pool implementation but it seems very unsafe if the 
class is redefined and relies on an implementation detail in 
the JVM that can change.   I will have more comments once I 
look more at the jvmti specification.


thanks,
Coleen

On 11/24/15 9:48 AM, Konstantin Shefov wrote:

Hello

Please, review modified webrev.

I have added methods
getNameAndTypeRefIndexAt(int index) - to get name and type 
entry index from a method, field or indy entry index;
getClassRefIndexAt(int index) - to get class entry index 
from a method or field entry index;


I removed previously added method
getInvokedynamicRefInfoAt(int index) - as it is no more 
needed now.


New webrev:
Webrev hotspot: 
http://cr.openjdk.java.net/~kshefov/8141615/hotspot/webrev.01 <http://cr.openjdk.java.net/%7Ekshefov/8141615/hot

Re: RFR JDK-8141491: Unaligned memory access in Bits.c

[Coleen Phillimore]

Sending to core-libs mailing list.

On 11/25/15 2:19 PM, Alexander Smundak wrote:

Please take a look at
http://cr.openjdk.java.net/~asmundak/8141491/jdk/webrev.00
that fixes the problem.

It utilizes the ability of some (GCC and Clang) to declare data
alignment explicitly.
I have verified it works on x86_64 Linux by running
jdk/test/java/nio/Buffer/Basic.java test

I need a sponsor.

Sasha

Re: Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

On 11/18/15 5:06 PM, Mandy Chung wrote:

On Nov 18, 2015, at 1:01 PM, Coleen Phillimore  
wrote:


One of the things that I'm struggling with is that StackFrameInfo contains both 
the collected information from walking the stack frames, method id, bci, 
mirror, version and cpref, and the digested information: interned string for 
class name, method name, line number and source file name.


method id, mirror, version and cpref are injected in StackFrameInfo.  I hope 
there is a way to make it conditional only if -XX:-MemberNameInThrowable is set 
(is it possible?)


That's really hard to do with the nice macros we have now in javaClasses.


-XX:-MemberNameInThrowable is temporary and disabled by default.  It is used 
for follow-on performance improvement as we discussed previously.   I still 
believe that there may be some low hanging fruit to reduce the initialization 
of MemberName for an already-linked method.We should aim to remove this 
flag in JDK 9 so that StackFrameInfo will have only MemberName and bci.


Given that that we were trying to stick to the original feature freeze 
date for 9, I don't think the performance of the MethodHandles code 
would make it in time.  There are some big problems with it, notably 
that it creates weak references for each MemberName and the GC people 
are not going to like that.   We have not to-date found a better 
solution for this to support redefinition.


I think if StackFrameInfo was trimmed to just what was needed for 
collecting the information during stack traces, it would be possible to 
make the new implementation performant enough to be low risk for 9 *and* 
would allow for removing the duplicated code in BacktraceBuilder.  This 
would be a very promising improvement!


The interned string for class name, method name, line number and source file 
name are requested lazily when StackFrame::getMethodName or other methods are 
called.  They are not eagerly allocated.


But the fields in StackFrameInfo are present for each element in the 
stack trace.  We had problems with GC scavenge times when we increased 
the size of the backtrace that we collect today.   The StackFrameInfo 
struct would be similarly sized if you didn't all the fields from 
StackTraceElement, so it would be good.



If this is to replace stack walking for exceptions, this will more than double 
the footprint of the information collected but rarely used. I don't understand 
why the digested information isn't still StackFrameElement?


If Throwable uses StackWalker, I expect it to use MemberName and 
-XX:-MemberNameInThrowable should be removed by that time.   Also VM no longer 
needs to fill in StackTraceElement as it should fill in StackFrameInfo.   
java_lang_StackTraceElement in javaClasses.[hc]pp can be removed at an 
appropriate time :)


I don't know why StackTraceElement should be removed.  What's wrong with 
StackTraceElement?


Throwable backtrace will keep an array of StackFrameInfo, one element per 
frame.  StackFrameInfo only captures the MemberName + bci.


Right (or the combination of things that we save now in the backtraces 
for efficiency).




When Throwable::getStackTraceElements() or printStackTrace() is called, the VM 
will allocate the intern strings for those names and fill in StackFrameInfo.  
Then convert them to StackTraceElement[] and throw away StackFrameInfo[].   
This is just the current implementation.   I expect further optimization can be 
done in the JDK side about StackTraceElement and StackFrameInfo.


This sounds really inefficient!  Why not fill in StackTraceElement 
directly?  And keep it?


Even in Java, having one class represent two different things isn't very 
object oriented.



That's just a high level comment.  I haven't read the java code yet for the 
rationale why this type is used for two different things.


The way I implement it is to prepare Throwable backtrace + StackTraceElement be 
replaced by StackFrameInfo in the VM.

The VM fills in StackFrameInfo for StackWalker.   When Throwable switches to 
use StackWalker, VM doesn’t need to know anything about StackTraceElement.


I don't see the advantage of this. 
java_lang_StackFrameInfo::fill_methodInfo() could just fill in a Java 
allocated array of StackTraceElement instead.  Again, making 
StackFrameInfo so large will have footprint and GC performance 
implications when it's almost always thrown away.  I included GC in the 
mailing list.  Hopefully with enough context if they want to comment.


thanks,
Coleen



Mandy

Re: Code Review for JEP 259: Stack-Walking API

[Coleen Phillimore]

One of the things that I'm struggling with is that StackFrameInfo 
contains both the collected information from walking the stack frames, 
method id, bci, mirror, version and cpref, and the digested information: 
interned string for class name, method name, line number and source file 
name.


If this is to replace stack walking for exceptions, this will more than 
double the footprint of the information collected but rarely used.  I 
don't understand why the digested information isn't still StackFrameElement?


That's just a high level comment.  I haven't read the java code yet for 
the rationale why this type is used for two different things.


Coleen

On 11/16/15 7:13 PM, Mandy Chung wrote:

I’d like to get the code review done by this week.

I renamed the static factory method from create to getInstance since “create” 
implies to create a new instance but the method returns a cached instance 
instead.  I also changed the spec of getCallerClass per [1].  There is not much 
change since webrev.01.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.02

javadoc:
http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/

Mandy
[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036589.html

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Coleen Phillimore]

On 11/2/15 1:57 PM, Ioi Lam wrote:



On 10/30/15 1:31 PM, Coleen Phillimore wrote:



On 10/30/15 4:18 PM, Karen Kinnear wrote:

Coleen,

Question for you below please ...
On Oct 30, 2015, at 3:44 PM, Coleen Phillimore 
 wrote:



Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html 



You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. 
ie. // SHARE_VM_MEMORY_CLASSLISTPARSER_HPP


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html 



  33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), 
THREAD);


This doesn't make sense.   If you want a permanent symbol, it 
doesn't need to get un-reference counted with the TempNewSymbol 
destructor.
Thank you for chiming in on this one - I wanted your opinion here. 
(this code used to be in MetaspaceShared::

preload_and_dump and I think was wrong there as well).
My understanding is that it is a TempNewSymbol that he wants, so he 
should call SymbolTable::new_symbol.
The code creates a (temporary) symbol for the name, and then calls 
SystemDictionary::resolve_or_null, which if it
succeeds will walk through the classFileParser which will create a 
permanent symbol for the class name,
via the symbol_alloc_batch handling. That would be consistent with 
the TempNewSymbol call in
SystemDictionary::resolve_or_null as well as in parse_stream - all 
places dealing with the class name

before doing classfile parsing.

Does that make sense?


Yes, this makes sense.  The symbol shouldn't be permanent.   Ioi can 
test this by putting a strange long name in the classlist file and 
make sure it doesn't make it out to the shared archive, since I think 
-Xshare:dump cleans out the SymbolTable before dumping.


After changing to use new_symbol instead of new_permanent_symbol, I 
ran -Xshare:dump:


$ java -Xshare:dump
Preload Warning: Cannot find sun/text/normalizer/UnicodeMatcher
...
total   :  13063134 [100.0% of total] out of  27385856 bytes [47.7% used]

$ strings images/jdk/lib/i386/hotspot/classes.jsa | grep 
sun/text/normalizer/UnicodeMatcher

sun/text/normalizer/UnicodeMatcher

So the unused symbols are not removed. Anyway, I have filed a separate 
issue for this: https://bugs.openjdk.java.net/browse/JDK-8141207


I just realized that the reason that it doesn't clean out unused symbols 
is that it would leave gaps in the Metaspace memory where they're 
stored, so there wouldn't be much point.


Coleen



Thanks
- Ioi



Coleen


thanks,
Karen

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html 



+// Make sure we have an entry in the SystemDictionary on success


This assert code is a copy of some code elsewhere.  Can you make it 
a function that they boh can call?

Can you also comment the raw #endif's to what they're endifing?

Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future 
enhancements.
[4] Add new APIs in the class loading code to support Oracle 
CDS enhancements.


Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Coleen Phillimore]

On 10/30/15 4:18 PM, Karen Kinnear wrote:

Coleen,

Question for you below please ...

On Oct 30, 2015, at 3:44 PM, Coleen Phillimore  
wrote:


Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html

You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. ie. // 
SHARE_VM_MEMORY_CLASSLISTPARSER_HPP

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html

  33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), THREAD);

This doesn't make sense.   If you want a permanent symbol, it doesn't need to 
get un-reference counted with the TempNewSymbol destructor.

Thank you for chiming in on this one - I wanted your opinion here. (this code 
used to be in MetaspaceShared::
preload_and_dump and I think was wrong there as well).
My understanding is that it is a TempNewSymbol that he wants, so he should call 
SymbolTable::new_symbol.
The code creates a (temporary) symbol for the name, and then calls 
SystemDictionary::resolve_or_null, which if it
succeeds will walk through the classFileParser which will create a permanent 
symbol for the class name,
via the symbol_alloc_batch handling. That would be consistent with the 
TempNewSymbol call in
SystemDictionary::resolve_or_null as well as in parse_stream - all places 
dealing with the class name
before doing classfile parsing.

Does that make sense?


Yes, this makes sense.  The symbol shouldn't be permanent.   Ioi can 
test this by putting a strange long name in the classlist file and make 
sure it doesn't make it out to the shared archive, since I think 
-Xshare:dump cleans out the SymbolTable before dumping.


Coleen


thanks,
Karen


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html

+// Make sure we have an entry in the SystemDictionary on success


This assert code is a copy of some code elsewhere.  Can you make it a function 
that they boh can call?
Can you also comment the raw #endif's to what they're endifing?

Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future enhancements.
[4] Add new APIs in the class loading code to support Oracle CDS 
enhancements.

Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: RFR (M) 8140802 - Clean up and refactor of class loading code for CDS

[Coleen Phillimore]

Hi Ioi,
This is a manageable code change.

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classListParser.hpp.html

You forward declare Klass* but don't use it in this header file.
Also can you add a comment to #endif  to say what it's endifing. ie. // 
SHARE_VM_MEMORY_CLASSLISTPARSER_HPP


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/classLoaderExt.cpp.html

  33   TempNewSymbol class_name_symbol = 
SymbolTable::new_permanent_symbol(parser->current_class_name(), THREAD);

This doesn't make sense.   If you want a permanent symbol, it doesn't 
need to get un-reference counted with the TempNewSymbol destructor.


http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/hotspot/src/share/vm/classfile/systemDictionary.cpp.udiff.html

+// Make sure we have an entry in the SystemDictionary on success


This assert code is a copy of some code elsewhere.  Can you make it a 
function that they boh can call?

Can you also comment the raw #endif's to what they're endifing?

Otherwise, this looks okay.

Coleen


On 10/30/15 1:00 PM, Ioi Lam wrote:

Please review the following fix:

http://cr.openjdk.java.net/~iklam/8140802-cds-refactoring.v01/

Bug: Clean up and refactor of class loading code for CDS

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

Summary of fix:

We need to clean up and refactor the class loading code in order
to support CDS in JDK9

[1] Remove code that has been made obsolete by the module changes
(such as supporting code used for meta-index file)
[2] Add new whitebox API to be used by CDS-related tests.
[3] Refactor the parsing of classlist files for future enhancements.
[4] Add new APIs in the class loading code to support Oracle CDS 
enhancements.


Tests:

JPRT
RBT - with same set of tests as hs-rt nightly

Thanks
- Ioi

Re: Please review: 8066185: VM crashed with SIGSEGV VirtualMemoryTracker::add_reserved_region

[Coleen Phillimore]

Kumar,

This looks good to me.  I didn't review all the changes in the test very 
carefully, so someone else should vouch for that.


Thank you for fixing this!

Coleen

On 2/23/15, 9:09 PM, David Holmes wrote:

Hi Kumar,

On 24/02/2015 8:14 AM, Kumar Srinivasan wrote:

Hello,

Please review the fix for the above issue.

Webrev:
http://cr.openjdk.java.net/~ksrini/8066185/webrev.00/

The fix is self explanatory, as for the test I have done the following:


I found the comment:

  /*
+  * Since this is a VM flag, we need to ensure it is not an
+  * application argument, meaning the argument must precede
+  * the main class or those flags that invoke the VM directly.
+  */

a bit confusing - specifically the "or those flags that invoke the VM 
directly". Took me while to realize that all the args you treat 
specially (-version, -h, -jar etc) are all "terminal" arguments - 
either the launcher stops looking at args after the given arg, or all 
following args must be for the application, not the launcher or VM. I 
would have expressed this as:


  /*
   * Since this must be a VM flag we stop processing once we see
   * an argument the launcher would not have processed beyond (such
   * as -version or -h), or an argument that indicates the following
   * arguments are for the application (i.e. the main class name, or
   * the -jar argument).
   */


a. refactored it from a single longish test to sub-tests for 
readability.

b. added new sub-test for NMT Argument Processing checks.


Can you please update the @summary for that test. It seems the OSX 
specific test was hijacked for NMT arg testing and the summary was 
never updated to reflect that.


Thanks,
David


Thanks
Kumar

Re: RFR: 8042418: Remove JVM_FindClassFromClassLoader

[Coleen Phillimore]

Thanks, David.
Coleen

On 12/13/14, 5:47 PM, David Holmes wrote:

Looks okay to me Coleen.

Thanks,
David

On 13/12/2014 6:59 AM, Coleen Phillimore wrote:

Summary: The function has been replaced so is no longer used.

This function was replaced with a better FindClassFromCaller function.
The compatibility request (CCC) was approved.

open webrev at http://cr.openjdk.java.net/~coleenp/8042418_jdk/
open webrev at http://cr.openjdk.java.net/~coleenp/8042418_hs
bug link https://bugs.openjdk.java.net/browse/JDK-8042418

Tested with jdk_core jtreg tests on linux/x64.

Thanks,
Coleen

RFR: 8042418: Remove JVM_FindClassFromClassLoader

[Coleen Phillimore]

Summary: The function has been replaced so is no longer used.

This function was replaced with a better FindClassFromCaller function.   
The compatibility request (CCC) was approved.


open webrev at http://cr.openjdk.java.net/~coleenp/8042418_jdk/
open webrev at http://cr.openjdk.java.net/~coleenp/8042418_hs
bug link https://bugs.openjdk.java.net/browse/JDK-8042418

Tested with jdk_core jtreg tests on linux/x64.

Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Thanks, Mandy!

Coleen

On 9/8/14, 6:59 PM, Mandy Chung wrote:

Thumbs up.

Mandy

On 9/5/2014 12:55 PM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the 
jdk9 code for 3 months without any problems.


The JDK changes hg imported cleanly.  The Hotspot change needed a 
hand merge for create_mirror call in klass.cpp.


http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

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

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also 
ran jck java_lang tests with only the hotspot change.  The hotspot 
change can be tested separately from the jdk change (but not the 
other way around).


Thanks,
Coleen

Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Thanks David!
Coleen

On 9/7/14, 9:38 PM, David Holmes wrote:

Looks okay to me.

David

On 6/09/2014 5:55 AM, Coleen Phillimore wrote:

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9
code for 3 months without any problems.

The JDK changes hg imported cleanly.  The Hotspot change needed a hand
merge for create_mirror call in klass.cpp.

http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

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

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran
jck java_lang tests with only the hotspot change.  The hotspot change
can be tested separately from the jdk change (but not the other way
around).

Thanks,
Coleen

[8u40] RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Summary: Add classLoader to java/lang/Class instance for fast access

This is a backport request for 8u40.   This change has been in the jdk9 
code for 3 months without any problems.


The JDK changes hg imported cleanly.  The Hotspot change needed a hand 
merge for create_mirror call in klass.cpp.


http://cr.openjdk.java.net/~coleenp/6642881_8u40_jdk/
http://cr.openjdk.java.net/~coleenp/6642881_8u40_hotspot/

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

Ran jdk_core jtreg tests in jdk with both jdk/hotspot changes. Also ran 
jck java_lang tests with only the hotspot change.  The hotspot change 
can be tested separately from the jdk change (but not the other way around).


Thanks,
Coleen

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

Hi Mandy,

The componentType field is the last one that I'm planning on moving out 
for now, so I'd like to keep the code as is.  If more are added because 
of more performance opportunities, I think we can revisit this.


I agree with Doug that we don't want any more special code like this in 
the JVM to disable these optimizations if they are ever implemented.


Thank you for reviewing the code.
Coleen

On 7/2/14, 2:26 AM, Mandy Chung wrote:


On 6/30/2014 9:51 PM, Christian Thalinger wrote:
On Jun 30, 2014, at 5:50 PM, Coleen Phillimore 
 wrote:



On 6/30/14, 3:50 PM, Christian Thalinger wrote:

  private Class(ClassLoader loader) {
  // Initialize final field for classLoader.  The 
initialization value of non-null
  // prevents future JIT optimizations from assuming this 
final field is null.

  classLoader = loader;
+componentType = null;
  }

Are we worried about the same optimization?
Hi,  I've decided to make them consistent and add another parameter 
to the Class constructor.


http://cr.openjdk.java.net/~coleenp/8047737_jdk_2/


The jdk change looks okay while I am beginning to think whether we 
really want to keep expanding this constructor to deal with this 
future JIT optimization (you will be moving more fields out from the 
VM to java.lang.Class).


There are places in JDK initializing the final fields to null while 
the final field value is overridden via native/VM - e.g. System.in, 
System.out, etc.  I would prefer reverting the classLoader constructor 
change to expanding the constructor for any new field being added.  
Handle it (and other places in JDK) when such JIT optimization comes.


Mandy

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

On 7/2/14, 8:41 AM, Peter Levart wrote:

On 07/02/2014 02:38 PM, Peter Levart wrote:

On 07/02/2014 02:22 PM, Peter Levart wrote:

On 07/02/2014 08:26 AM, Mandy Chung wrote:


On 6/30/2014 9:51 PM, Christian Thalinger wrote:
On Jun 30, 2014, at 5:50 PM, Coleen Phillimore 
 wrote:



On 6/30/14, 3:50 PM, Christian Thalinger wrote:

  private Class(ClassLoader loader) {
  // Initialize final field for classLoader. The 
initialization value of non-null
  // prevents future JIT optimizations from assuming 
this final field is null.

  classLoader = loader;
+componentType = null;
  }

Are we worried about the same optimization?
Hi,  I've decided to make them consistent and add another 
parameter to the Class constructor.


http://cr.openjdk.java.net/~coleenp/8047737_jdk_2/


The jdk change looks okay while I am beginning to think whether we 
really want to keep expanding this constructor to deal with this 
future JIT optimization (you will be moving more fields out from 
the VM to java.lang.Class).


There are places in JDK initializing the final fields to null while 
the final field value is overridden via native/VM - e.g. System.in, 
System.out, etc.  I would prefer reverting the classLoader 
constructor change to expanding the constructor for any new field 
being added.  Handle it (and other places in JDK) when such JIT 
optimization comes.


Mandy



What about:


private Class() {
classLoader = none();
componentType = none();
...
}

private  T none() { throw new Error(); }


I think this should be resistant to future optimizations.


And you could even remove the special-casing in 
AccessibleObject.setAccessible0() then.


Regards, Peter


I take it back. Such java.lang.Class instance would still be 
constructed and GC will see it.


The setAccessible0 check is still needed because we do other things to 
the mirror inside the jvm.

Coleen







Regards, Peter

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

Thank you!
Coleen

On 7/1/14, 12:51 AM, Christian Thalinger wrote:

On Jun 30, 2014, at 5:50 PM, Coleen Phillimore  
wrote:


On 6/30/14, 3:50 PM, Christian Thalinger wrote:

  private Class(ClassLoader loader) {
  // Initialize final field for classLoader.  The initialization value 
of non-null
  // prevents future JIT optimizations from assuming this final field 
is null.
  classLoader = loader;
+componentType = null;
  }

Are we worried about the same optimization?

Hi,  I've decided to make them consistent and add another parameter to the 
Class constructor.

http://cr.openjdk.java.net/~coleenp/8047737_jdk_2/

Thanks.


Thanks,
Coleen

+  compute_optional_offset(_component_mirror_offset,
+ klass_oop, vmSymbols::componentType_name(),
+ vmSymbols::class_signature());

Is there a followup cleanup to make it non-optional?  Or, are you waiting for 
JPRT to be able to push hotspot and jdk changes together?

On Jun 30, 2014, at 5:42 AM, Coleen Phillimore mailto:coleen.phillim...@oracle.com>> wrote:


On 6/30/14, 1:55 AM, David Holmes wrote:

Hi Coleen,

Your webrev links are to internal locations.

Sorry, I cut/pasted the wrong links.  They are:

http://cr.openjdk.java.net/~coleenp/8047737_jdk/ 
<http://cr.openjdk.java.net/%7Ecoleenp/8047737_jdk/>
http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

and the full version

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

Thank you for pointing this out David.

Coleen


David

On 28/06/2014 5:24 AM, Coleen Phillimore wrote:

Summary: Add field in java.lang.Class for componentType to simplify oop
processing and intrinsics in JVM

This is part of ongoing work to clean up oop pointers in the metadata
and simplify the interface between the JDK j.l.C and the JVM. There's a
performance benefit at the end of all of this if we can remove all oop
pointers from metadata.   mirror in Klass is the only one left after
this full change.

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

There are a couple steps to this change because Hotspot testing is done
with promoted JDKs.  The first step is this webrev:

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove
ArrayKlass::_component_mirror will be changed under a new bug id.

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will occur to
remove the function JVM_GetComponentType.

Performance testing was done that shows no difference in performance.
The isArray() call is a compiler intrinsic which is now called instead
of getComponentType, which was recognized as a compiler intrinsic.

JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
tests were performed on both the change requested (1st one) and the full
change.

hotspot NSK tests were run on the hotspot-only change with a promoted JDK.

Please send your comments.

Thanks,
Coleen

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

On 6/30/14, 3:50 PM, Christian Thalinger wrote:

  private Class(ClassLoader loader) {
  // Initialize final field for classLoader.  The initialization value 
of non-null
  // prevents future JIT optimizations from assuming this final field 
is null.
  classLoader = loader;
+componentType = null;
  }

Are we worried about the same optimization?


Hi,  I've decided to make them consistent and add another parameter to 
the Class constructor.


http://cr.openjdk.java.net/~coleenp/8047737_jdk_2/

Thanks,
Coleen


+  compute_optional_offset(_component_mirror_offset,
+ klass_oop, vmSymbols::componentType_name(),
+ vmSymbols::class_signature());

Is there a followup cleanup to make it non-optional?  Or, are you 
waiting for JPRT to be able to push hotspot and jdk changes together?


On Jun 30, 2014, at 5:42 AM, Coleen Phillimore 
mailto:coleen.phillim...@oracle.com>> 
wrote:




On 6/30/14, 1:55 AM, David Holmes wrote:

Hi Coleen,

Your webrev links are to internal locations.


Sorry, I cut/pasted the wrong links.  They are:

http://cr.openjdk.java.net/~coleenp/8047737_jdk/ 
<http://cr.openjdk.java.net/%7Ecoleenp/8047737_jdk/>

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

and the full version

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

Thank you for pointing this out David.

Coleen



David

On 28/06/2014 5:24 AM, Coleen Phillimore wrote:

Summary: Add field in java.lang.Class for componentType to simplify oop
processing and intrinsics in JVM

This is part of ongoing work to clean up oop pointers in the metadata
and simplify the interface between the JDK j.l.C and the JVM. There's a
performance benefit at the end of all of this if we can remove all oop
pointers from metadata.   mirror in Klass is the only one left after
this full change.

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

There are a couple steps to this change because Hotspot testing is done
with promoted JDKs.  The first step is this webrev:

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove
ArrayKlass::_component_mirror will be changed under a new bug id.

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will 
occur to

remove the function JVM_GetComponentType.

Performance testing was done that shows no difference in performance.
The isArray() call is a compiler intrinsic which is now called instead
of getComponentType, which was recognized as a compiler intrinsic.

JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
tests were performed on both the change requested (1st one) and the 
full

change.

hotspot NSK tests were run on the hotspot-only change with a 
promoted JDK.


Please send your comments.

Thanks,
Coleen

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

On 6/30/14, 3:50 PM, Christian Thalinger wrote:

  private Class(ClassLoader loader) {
  // Initialize final field for classLoader.  The initialization value 
of non-null
  // prevents future JIT optimizations from assuming this final field 
is null.
  classLoader = loader;
+componentType = null;
  }

Are we worried about the same optimization?


I don't know if I was justified in worrying about the optimization in 
the first place.   Since getComponentType() is conditional, I wasn't 
worried.


But it should be consistent.  Maybe I should revert the classLoader 
constructor change, now that I fixed all the tests not to care. What do 
people think?


+  compute_optional_offset(_component_mirror_offset,
+ klass_oop, vmSymbols::componentType_name(),
+ vmSymbols::class_signature());

Is there a followup cleanup to make it non-optional?  Or, are you 
waiting for JPRT to be able to push hotspot and jdk changes together?


Yes, please look at the _full webrev.  That has the non-optional changes 
in it and the follow on changes to remove getComponentType as an 
intrinsic in C2 (will file new RFE).  I really would like a compiler 
person to comment on it.


Thanks so much,
Coleen



On Jun 30, 2014, at 5:42 AM, Coleen Phillimore 
mailto:coleen.phillim...@oracle.com>> 
wrote:




On 6/30/14, 1:55 AM, David Holmes wrote:

Hi Coleen,

Your webrev links are to internal locations.


Sorry, I cut/pasted the wrong links.  They are:

http://cr.openjdk.java.net/~coleenp/8047737_jdk/ 
<http://cr.openjdk.java.net/%7Ecoleenp/8047737_jdk/>

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

and the full version

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

Thank you for pointing this out David.

Coleen



David

On 28/06/2014 5:24 AM, Coleen Phillimore wrote:

Summary: Add field in java.lang.Class for componentType to simplify oop
processing and intrinsics in JVM

This is part of ongoing work to clean up oop pointers in the metadata
and simplify the interface between the JDK j.l.C and the JVM. There's a
performance benefit at the end of all of this if we can remove all oop
pointers from metadata.   mirror in Klass is the only one left after
this full change.

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

There are a couple steps to this change because Hotspot testing is done
with promoted JDKs.  The first step is this webrev:

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove
ArrayKlass::_component_mirror will be changed under a new bug id.

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will 
occur to

remove the function JVM_GetComponentType.

Performance testing was done that shows no difference in performance.
The isArray() call is a compiler intrinsic which is now called instead
of getComponentType, which was recognized as a compiler intrinsic.

JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
tests were performed on both the change requested (1st one) and the 
full

change.

hotspot NSK tests were run on the hotspot-only change with a 
promoted JDK.


Please send your comments.

Thanks,
Coleen

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

Thank you, Fred, and thanks for correcting the link below.
Coleen

On 6/30/14, 10:27 AM, Frederic Parain wrote:



On 30/06/2014 14:42, Coleen Phillimore wrote:


On 6/30/14, 1:55 AM, David Holmes wrote:

Hi Coleen,

Your webrev links are to internal locations.


Sorry, I cut/pasted the wrong links.  They are:

http://cr.openjdk.java.net/~coleenp/8047737_jdk/
http://cr.openjdk.java.net/~coleenp/8047737_hotspot/


First step looks good.


and the full version

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/


The correct link to the full version seems to be:
http://cr.openjdk.java.net/~coleenp/8047737_hotspot_full/

I haven't reviewed the full version yet.

Fred


Thank you for pointing this out David.

Coleen



David

On 28/06/2014 5:24 AM, Coleen Phillimore wrote:
Summary: Add field in java.lang.Class for componentType to simplify 
oop

processing and intrinsics in JVM

This is part of ongoing work to clean up oop pointers in the metadata
and simplify the interface between the JDK j.l.C and the JVM. 
There's a

performance benefit at the end of all of this if we can remove all oop
pointers from metadata.   mirror in Klass is the only one left after
this full change.

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

There are a couple steps to this change because Hotspot testing is 
done

with promoted JDKs.  The first step is this webrev:

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove
ArrayKlass::_component_mirror will be changed under a new bug id.

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will 
occur to

remove the function JVM_GetComponentType.

Performance testing was done that shows no difference in performance.
The isArray() call is a compiler intrinsic which is now called instead
of getComponentType, which was recognized as a compiler intrinsic.

JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
tests were performed on both the change requested (1st one) and the 
full

change.

hotspot NSK tests were run on the hotspot-only change with a promoted
JDK.

Please send your comments.

Thanks,
Coleen

Re: RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

On 6/30/14, 1:55 AM, David Holmes wrote:

Hi Coleen,

Your webrev links are to internal locations.


Sorry, I cut/pasted the wrong links.  They are:

http://cr.openjdk.java.net/~coleenp/8047737_jdk/
http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

and the full version

http://cr.openjdk.java.net/~coleenp/8047737_hotspot/

Thank you for pointing this out David.

Coleen



David

On 28/06/2014 5:24 AM, Coleen Phillimore wrote:

Summary: Add field in java.lang.Class for componentType to simplify oop
processing and intrinsics in JVM

This is part of ongoing work to clean up oop pointers in the metadata
and simplify the interface between the JDK j.l.C and the JVM. There's a
performance benefit at the end of all of this if we can remove all oop
pointers from metadata.   mirror in Klass is the only one left after
this full change.

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

There are a couple steps to this change because Hotspot testing is done
with promoted JDKs.  The first step is this webrev:

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove
ArrayKlass::_component_mirror will be changed under a new bug id.

http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will occur to
remove the function JVM_GetComponentType.

Performance testing was done that shows no difference in performance.
The isArray() call is a compiler intrinsic which is now called instead
of getComponentType, which was recognized as a compiler intrinsic.

JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8
tests were performed on both the change requested (1st one) and the full
change.

hotspot NSK tests were run on the hotspot-only change with a promoted 
JDK.


Please send your comments.

Thanks,
Coleen

RFR 8047737 Move array component mirror to instance of java/lang/Class

[Coleen Phillimore]

Summary: Add field in java.lang.Class for componentType to simplify oop 
processing and intrinsics in JVM


This is part of ongoing work to clean up oop pointers in the metadata 
and simplify the interface between the JDK j.l.C and the JVM.  There's a 
performance benefit at the end of all of this if we can remove all oop 
pointers from metadata.   mirror in Klass is the only one left after 
this full change.


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

There are a couple steps to this change because Hotspot testing is done 
with promoted JDKs.  The first step is this webrev:


http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_jdk/
http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot/

When the JDK is promoted, the code to remove 
ArrayKlass::_component_mirror will be changed under a new bug id.


http://oklahoma.us.oracle.com/~cphillim/webrev/8047737_hotspot_full

Finally, a compatibility request and licensee notification will occur to 
remove the function JVM_GetComponentType.


Performance testing was done that shows no difference in performance.  
The isArray() call is a compiler intrinsic which is now called instead 
of getComponentType, which was recognized as a compiler intrinsic.


JDK jtreg testing, hotspot jtreg testing, hotspot NSK testing and jck8 
tests were performed on both the change requested (1st one) and the full 
change.


hotspot NSK tests were run on the hotspot-only change with a promoted JDK.

Please send your comments.

Thanks,
Coleen

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

On 6/24/14, 4:41 AM, Frederic Parain wrote:

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.


I removed this code.  There are no other instances of the macro 
BROKEN_JAVAC.  I update the copyrights when I commit the changeset.


http://cr.openjdk.java.net/~coleenp/6642881_jdk_5/

Thanks!
Coleen


Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 




Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class 
loader.



There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs
"getClassLoader"
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Mikael,   see below.
On 6/24/14, 9:48 AM, Mikael Gerdin wrote:

On Tuesday 24 June 2014 08.51.18 Coleen Phillimore wrote:

Hi Peter,

On 6/24/14, 4:23 AM, Peter Levart wrote:

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:

Please review a change to the JDK code for adding classLoader field
to the instances of java/lang/Class.  This change restricts
reflection from changing access to the classLoader field.  In the
spec, AccessibleObject.setAccessible() may throw SecurityException if
the accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObje
ct.html#setAccessible(boolean)


Only AccessibleObject.java has changes from the previous version of
this change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

Hi Coleen,

So hackers are prevented from hacking...

Out of curiosity, what would happen if someone changed the classLoader
field of some Class object? I guess VM still has it's own notion of
the class' class loader, right? Only the code that directly uses the
Class.getClassLoader() (and Unsafe.defineClass0) methods would be
affected...

True, Class.getClassLoader() calls would be affected but we may use this
call for security checks.  I'm not really an expert on this, but we
thought it wouldn't be safe to allow user access to classLoader.


While we're at it, does this change in any way affect the GC logic?
Will GC now navigate the classLoader field during marking but
previously didn't ? Will this have any GC performance penalty ?

I actually ran this through our performance testing and got a good
improvement in one of the scimark benchmarks for no reason I could
explain (and lost the results), but none of the other tests were
affected.  GC would have to mark this new field for full collections but
not young collections because it's only set during initialization.   I
wouldn't expect this field to have any negative performance for GC.

Peter has a good point actually. If the classLoader field is trustworthy we
can get rid of some code in InstanceMirrorKlass::{oop_follow_contents,
oop_oop_iterate} which currently picks up the class loader through the Klass*
field injected into java/lang/Class. It would be a nice simplification if the
ClassLoader objects were the only places where the GC would need to find the
link to the ClassLoaderData metadata.

But that won't work because anonymous (as in Unsafe.defineAnonymousClass)
classes don't interact with class loaders in the same way and have their own
ClassLoaderData objects.


Yes, one version of this patch removed class_loader_data from 
InstanceKlass so we'd be space-neutral but since there isn't a 1-1 
relationship to class_loader and ClassLoaderData for Unsafe anonymous 
classes, it doesn't work.  We might think of a way around this because 
we want to go in this direction.


Thanks,
Coleen


/Mikael


Thanks,
Coleen


Regards, Peter


On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:

On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore

 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which
should go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class
loader.

There is sun.reflect.Reflection.registerFieldsToFilter() that
hides a field from being found using reflection. It might very
well be the case that private and final is enough, I haven’t
thought this through 100%. On the other hand, is there a reason
to give users access through the field instead of having to use
Class.getClassLoader()?

There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs
"getClassLoader"
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.

I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoa

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Fred,

Thank you for finding this.  Yes, I meant to clean this up with the bug 
to remove JVM_GetClassLoader but I should remove this with this change 
instead, since the other change will be in hotspot only.

Yes, it's dead code.

Thanks!
Coleen

On 6/24/14, 4:41 AM, Frederic Parain wrote:

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.

Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 




Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection. It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class 
loader.



There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs
"getClassLoader"
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Hi Peter,

On 6/24/14, 4:23 AM, Peter Levart wrote:

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field 
to the instances of java/lang/Class.  This change restricts 
reflection from changing access to the classLoader field.  In the 
spec, AccessibleObject.setAccessible() may throw SecurityException if 
the accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen


Hi Coleen,

So hackers are prevented from hacking...

Out of curiosity, what would happen if someone changed the classLoader 
field of some Class object? I guess VM still has it's own notion of 
the class' class loader, right? Only the code that directly uses the 
Class.getClassLoader() (and Unsafe.defineClass0) methods would be 
affected...


True, Class.getClassLoader() calls would be affected but we may use this 
call for security checks.  I'm not really an expert on this, but we 
thought it wouldn't be safe to allow user access to classLoader.




While we're at it, does this change in any way affect the GC logic? 
Will GC now navigate the classLoader field during marking but 
previously didn't ? Will this have any GC performance penalty ?


I actually ran this through our performance testing and got a good 
improvement in one of the scimark benchmarks for no reason I could 
explain (and lost the results), but none of the other tests were 
affected.  GC would have to mark this new field for full collections but 
not young collections because it's only set during initialization.   I 
wouldn't expect this field to have any negative performance for GC.


Thanks,
Coleen



Regards, Peter



On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class 
loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason 
to give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs 
"getClassLoader"

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but 
since you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that 
either if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

On 6/23/14, 9:36 PM, Mandy Chung wrote:

Coleen,

On 6/23/2014 4:45 PM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field 
to the instances of java/lang/Class.  This change restricts 
reflection from changing access to the classLoader field.  In the 
spec, AccessibleObject.setAccessible() may throw SecurityException if 
the accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) 



Only AccessibleObject.java has changes from the previous version of 
this change.




This change looks reasonable.As a side note:  Joel mentions about 
the mechanism to hide class members from reflection.   I discussed 
with Joel offline before he went on parental leave and suggest that we 
should revisit the two mechanisms that both effectively disallow 
access to private members in the future.


Thanks, Mandy.  Yes, let me know what you come up with and we can 
improve this.  Thank you for the help fixing this bug.


Coleen



Mandy


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which 
should go away in 9 but …).
I don't know how to hide the field from reflection. It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class 
loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that 
hides a field from being found using reflection. It might very 
well be the case that private and final is enough, I haven’t 
thought this through 100%. On the other hand, is there a reason 
to give users access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs 
"getClassLoader"

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but 
since you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that 
either if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Coleen Phillimore]

Please review a change to the JDK code for adding classLoader field to 
the instances of java/lang/Class.  This change restricts reflection from 
changing access to the classLoader field.  In the spec, 
AccessibleObject.setAccessible() may throw SecurityException if the 
accessibility of an object may not be changed:


http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)

Only AccessibleObject.java has changes from the previous version of this 
change.


open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung  wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore 
 wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
Have you considered hiding the Class.classLoader field from 
reflection? I’m not sure it is necessary but I’m not to keen on 
the idea of people poking at this field with Unsafe (which should 
go away in 9 but …).
I don't know how to hide the field from reflection.  It's a 
private final field so you need to get priviledges to make it 
setAccessible.  If you mean injecting it on the JVM side, the 
reason for this change is so that the JIT compilers can inline 
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that hides 
a field from being found using reflection. It might very well be 
the case that private and final is enough, I haven’t thought this 
through 100%. On the other hand, is there a reason to give users 
access through the field instead of having to use 
Class.getClassLoader()?



There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires "accessDeclaredMember"
permission although it's a different security check (vs 
"getClassLoader"

permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the "accessDeclaredMember" permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote, 
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since 
you could get the class loader through getClassLoader0() that 
shouldn’t be a new issue.


The second thing is that IIRC there are ways to set a final field 
after initialization. I’m not sure we need to care about that either 
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call 
setAccessible(true) on the Field object.


David
-



cheers
/Joel

Re: RFR (S) 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[Coleen Phillimore]

We didn't file any bugs because I don't remember finding anything 
specific, other than "gosh that code is scary" and "I wish we didn't 
have to do this".


If you find a null 'm' below and call m->print() is the method "obsolete"?

Coleen

On 4/30/14, 8:24 PM, Jeremy Manson wrote:

Did the new bugs get filed for this?

I'm triggering this check with a redefined class (from the 
bootclasspath, if that matters).  To investigate it a little, I 
instrumented StackTraceElement::create thus:


oop java_lang_StackTraceElement::create(methodHandle method, int bci, 
TRAPS) {

  Handle mirror (THREAD, method->method_holder()->java_mirror());
  int method_id = method->method_idnum();

  InstanceKlass* holder = method->method_holder();
  Method* m = holder->method_with_idnum(method_id);
  Method* mp = holder->find_method(method->name(), method->signature());
  method->print_name();
  fprintf(stderr, " method = %p id = %d method' = %p \n", m, 
method_id, mp);
  return create(mirror, method_id, method->constants()->version(), 
bci, THREAD);

}

m is null, and mp isn't.  method->print_name works fine.  This makes 
me feel that the idnum array is out of date somehow.  Before I go down 
the rabbit hole and try to figure out why that is, does someone else 
know why?


Thanks!

Jeremy



On Thu, Oct 3, 2013 at 11:02 AM, Coleen Phillimore 
mailto:coleen.phillim...@oracle.com>> 
wrote:


Summary: Redefined class in stack trace may not be found by
method_idnum so handle null.

This is a simple change.  I had another change to save the method
name (as u2) in the backtrace, but it's not worth the extra
footprint in backtraces for this rare case.

The root problem was that we save method_idnum in the backtrace
(u2) instead of Method* to avoid Method* from being redefined and
deallocated.  I made a change to
InstanceKlass::method_from_idnum() to return null rather than the
last method in the list, which causes this crash. Dan and I went
down the long rabbit-hole of why method_idnum is changed for
obsolete methods and we think there's some cleanup and potential
bugs in this area.  But this is not that change.  I'll file
another bug to continue this investigation for jdk9 (or 8uN).

Staffan created a test - am including core-libs for the review
request.  Also tested with all of the vm testbase tests, mlvm
tests, and java/lang/instrument tests.

open webrev at http://cr.openjdk.java.net/~coleenp/8025238/
<http://cr.openjdk.java.net/%7Ecoleenp/8025238/>
bug link https://bugs.openjdk.java.net/browse/JDK-8025238

test case for jdk8 repository:

open webrev at http://cr.openjdk.java.net/~coleenp/8025238_jdk
<http://cr.openjdk.java.net/%7Ecoleenp/8025238_jdk>

Thanks,
Coleen

hg: jdk8/tl/jdk: 2 new changesets

[coleen . phillimore]

Changeset: 59f46f135584
Author:hseigel
Date:  2013-11-14 10:44 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/59f46f135584

8023041: The CDS classlist needs to be updated for JDK 8
Summary: Generate new classlists from JDK 8 classes
Reviewed-by: alanb, coleenp, hseigel
Contributed-by: ekaterina.pavl...@oracle.com

! make/tools/sharing/classlist.linux
! make/tools/sharing/classlist.macosx
! make/tools/sharing/classlist.solaris
! make/tools/sharing/classlist.windows

Changeset: f893901ba29c
Author:coleenp
Date:  2013-11-14 14:01 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f893901ba29c

Merge

Re: RFR (M) 8023041: The CDS classlist needs to be updated for JDK 8

[Coleen Phillimore]

Thanks, Alan.  I've looked at this and reviewed it too, so I'll push 
it.  Thanks Harold for sending it out.


Coleen

On 11/14/2013 10:03 AM, harold seigel wrote:

Hi Alan,

Thank you for the review.

Harold

On 11/13/2013 10:04 AM, Alan Bateman wrote:

On 13/11/2013 14:55, harold seigel wrote:

Hi,

Please review this fix, submitted by Ekaterina Pavlova, to update 
the CDS classlist files for JDK 8.


The classlist files were generated using the process described in 
jdk/make/tools/sharing/README.txt.  In addition, a checksum was 
included.


The open webrev is at: 
http://cr.openjdk.java.net/~hseigel/bug_8023041/ 



The bug is at: https://bugs.openjdk.java.net/browse/JDK-8023041

Additional information about this change, including testing 
information, is in the bug.
Good to see these sync'ed up. I assume this will remove a lot of 
warnings from the build too.


Assuming that the process to generate these was indeed followed then 
I'd suggest going ahead and just push it (as there isn't much we can 
actually review except to spot classes removed from the list that no 
longer exist).


-Alan.

hg: jdk8/tl/jdk: 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV

[coleen . phillimore]

Changeset: f581b72e3715
Author:sla
Date:  2013-10-21 23:32 -0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f581b72e3715

8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV
Summary: Redefined class in stack trace may not be found by method_idnum so 
handle null.
Reviewed-by: coleenp, dcubed, sspitsyn

! test/java/lang/instrument/RedefineMethodInBacktrace.sh
! test/java/lang/instrument/RedefineMethodInBacktraceApp.java
+ test/java/lang/instrument/RedefineMethodInBacktraceTargetB.java
+ test/java/lang/instrument/RedefineMethodInBacktraceTargetB_2.java