Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]
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
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]
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]
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]
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]
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]
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]
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]
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]
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]
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]
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(_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]
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
Re: RFR: JDK-8256844: Make NMT late-initializable [v2]
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]
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
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
Re: RFR: 8254598: StringDedupTable should use OopStorage
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(); > 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(); } 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]
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]
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
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
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]
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]
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]
> 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=2410=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2410=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]
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]
> 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=2410=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2410=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
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
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
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]
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]
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]
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]
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]
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
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=1783=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]
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]
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: , 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]
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]
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]
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
Re: RFR: 8246774: Record Classes (final) implementation [v3]
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()
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]
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
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
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
Re: RFR: JEP 359: Records (Preview) (full code)
(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
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
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.
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
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
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. ---
Re: RFR (S) 8173715: Remove FlatProfiler
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)
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)
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
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
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 <S,T> TestClass1(S,T) throws java.lang.Exception") + @ExpectedString( + "protected TestClass1(java.lang.Object,java.lang.Object) throws java.lang.Exception") protected <S, T> 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
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( "protectedTestClass1(S,T) throws java.lang.Exception") + @ExpectedString( + "protected TestClass1(java.lang.Object,java.lang.Object) throws java.lang.Exception") protectedTestClass1(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
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
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 <brent.christ...@oracle.com> 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
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 <brent.christ...@oracle.com> 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
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 <brent.christ...@oracle.com> 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
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
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
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 <coleen.phillim...@oracle.com> 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
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
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: ClassFileTransformer does not apply to anonymous classes
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
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 <konstantin.she...@oracle.com> 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 <konstantin.she...@oracle.com <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 <konstantin.she...@oracle.com <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.jav
Re: RFR JDK-8141491: Unaligned memory access in Bits.c
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
On 11/18/15 5:06 PM, Mandy Chung wrote: On Nov 18, 2015, at 1:01 PM, Coleen Phillimore <coleen.phillim...@oracle.com> 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
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
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 <coleen.phillim...@oracle.com> 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
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 <coleen.phillim...@oracle.com> 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
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
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
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
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()
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
Re: [8u40] RFR 6642881: Improve performance of Class.getClassLoader()
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
[8u40] RFR 6642881: Improve performance of Class.getClassLoader()
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
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 coleen.phillim...@oracle.com 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 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
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 coleen.phillim...@oracle.com 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
Thank you! Coleen On 7/1/14, 12:51 AM, Christian Thalinger wrote: On Jun 30, 2014, at 5:50 PM, Coleen Phillimore coleen.phillim...@oracle.com 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 coleen.phillim...@oracle.com 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 6642881: Improve performance of Class.getClassLoader()
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 mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com 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()
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 mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com 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()
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 mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com 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()
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 mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com 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()
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 mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com 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
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 coleen.phillim...@oracle.com 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
Re: RFR (M) 8023041: The CDS classlist needs to be updated for JDK 8
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/ http://cr.openjdk.java.net/%7Ehseigel/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: 2 new changesets
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
hg: jdk8/tl/jdk: 8025238: nsk/jvmti/scenarios/bcinstr/BI04/bi04t002 crashed with SIGSEGV
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
RFR (S) 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. 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/ 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 Thanks, Coleen
hg: jdk8/tl/jdk: 2 new changesets
Changeset: cb74d71fd02f Author:hseigel Date: 2013-08-13 10:56 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/cb74d71fd02f 8022259: MakeClasslist is buggy and its README is out of date. Summary: Fixed bug in FOR loop and updated comments and README Reviewed-by: dholmes, alanb ! make/tools/sharing/README.txt ! make/tools/src/build/tools/makeclasslist/MakeClasslist.java Changeset: f9cf6ecf596c Author:coleenp Date: 2013-08-14 10:14 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f9cf6ecf596c Merge
hg: jdk8/tl/jdk: 7124706: enable RetransformBigClass.sh test when fix for 8013063 is promoted
Changeset: f18337edd201 Author:coleenp Date: 2013-06-07 22:15 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f18337edd201 7124706: enable RetransformBigClass.sh test when fix for 8013063 is promoted Summary: The code for this test is fixed now and integrated to TL repo and it passes now. Reviewed-by: alanb ! test/java/lang/instrument/MakeJAR4.sh ! test/java/lang/instrument/RetransformBigClass.sh
RFR: 7124706: enable RetransformBigClass.sh test when fix for 8013063 is promoted
Summary: The code for this test is fixed now and integrated to TL repo and it passes now. open webrev at http://cr.openjdk.java.net/~coleenp/7124706_jdk/ bug link at http://bugs.sun.com/view_bug.do?bug_id=7124706_jdk Thanks, Coleen
hg: jdk8/tl/jdk: 2 new changesets
Changeset: 7f9f69729934 Author:coleenp Date: 2013-04-17 12:50 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/7f9f69729934 8009531: Crash when redefining class with annotated method Summary: Add code to annotated methods and command line flags to the tests to verify bug above Reviewed-by: acorn, sspitsyn, dcubed, dholmes, alanb ! test/java/lang/instrument/RedefineMethodWithAnnotations.sh ! test/java/lang/instrument/RedefineMethodWithAnnotationsTarget.java ! test/java/lang/instrument/RedefineMethodWithAnnotationsTarget_2.java Changeset: 36f9e357b84b Author:coleenp Date: 2013-04-17 15:06 -0400 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/36f9e357b84b Merge
Re: RFR (S) 8009531: Crash when redefining class with annotated method
On 04/14/2013 11:29 PM, David Holmes wrote: On 15/04/2013 1:08 PM, David Holmes wrote: Hi Coleen, On 13/04/2013 3:45 AM, Coleen Phillimore wrote: Summary: Add annotations to the tests to verify bug above open webrev at http://cr.openjdk.java.net/~coleenp/8009531_jdk/ bug link at http://bugs.sun.com/view_bug.do?bug_id=8009531_jdk The Hotspot change is in tl repository now. Also, this has been reviewed by the hotspot group. Is the StressLdcRewrite essential to the test? (Aside: And why is that a product flag ??) I don't know why it's a product flag. I added IgnoreUnrecognizedVMOptions in case it changes though. Otherwise looks okay. Strike that. I missed what Alan pointed out. You are not actually adding the annotations. So why do we need the printlns? I added the printlns to make sure there are bytecodes to rewrite in the method. Coleen David David Thanks, Coleen
RFR (S) 8009531: Crash when redefining class with annotated method
Summary: Add annotations to the tests to verify bug above open webrev at http://cr.openjdk.java.net/~coleenp/8009531_jdk/ bug link at http://bugs.sun.com/view_bug.do?bug_id=8009531_jdk The Hotspot change is in tl repository now. Also, this has been reviewed by the hotspot group. Thanks, Coleen
Re: RFR (S) 8009531: Crash when redefining class with annotated method
Can you reply to me as well as the mailing list since I'm not on this mailing list? thanks, Coleen Original Message Subject: RFR (S) 8009531: Crash when redefining class with annotated method Date: Fri, 12 Apr 2013 13:45:30 -0400 From: Coleen Phillimore coleen.phillim...@oracle.com Reply-To: coleen.phillim...@oracle.com Organization: Oracle Corporation To: core-libs-dev@openjdk.java.net Summary: Add annotations to the tests to verify bug above open webrev at http://cr.openjdk.java.net/~coleenp/8009531_jdk/ bug link at http://bugs.sun.com/view_bug.do?bug_id=8009531 The Hotspot change is in tl repository now. Also, this has been reviewed by the hotspot group. Thanks, Coleen