On Mon, 1 Apr 2024 21:07:31 GMT, Vladimir Kozlov <k...@openjdk.org> wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Removed not_used state of nmethod Nice! We've wanted to clean up some interfaces between the CodeCache and the GC code by using nmethod closures instead of CodeBlob closures. This change (and the Sweeper removal) makes it possible to do those cleanups. I've made a superficial pass over the patch to and left a few comments. Most of those comments are things that would be nice to fix, but could also be left as follow-up RFEs (if they are deemed to be worthy ideas to pursue). src/hotspot/os/posix/signals_posix.cpp line 27: > 25: #include "precompiled.hpp" > 26: #include "code/codeCache.hpp" > 27: #include "code/nmethod.hpp" The include line needs to move down. src/hotspot/share/code/codeBlob.hpp line 77: > 75: // - data space > 76: > 77: enum CodeBlobKind : u1 { It will probably be safer to change this to an enum class, so that the compiler will help us if we mess up with the argument order when this is used in function calls. I see that this patch switches the parameter order of some functions, so I think it could be worth trying out. src/hotspot/share/code/codeBlob.hpp line 409: > 407: > 408: // GC/Verification support > 409: virtual void preserve_callee_argument_oops(frame fr, const RegisterMap > *reg_map, OopClosure* f) override { /* nothing to do */ } In the GC code we usually have either virtual OR override, but not both. Could we skip `virtual` here? Or does the compiler code usually use both? src/hotspot/share/code/codeBlob.hpp line 429: > 427: SingletonBlob( > 428: const char* name, > 429: CodeBlobKind kind, There's an alignment issue after this change. src/hotspot/share/code/codeCache.cpp line 1009: > 1007: int CodeCache::nmethod_count() { > 1008: int count = 0; > 1009: for (GrowableArrayIterator<CodeHeap*> heap = _nmethod_heaps->begin(); > heap != _nmethod_heaps->end(); ++heap) { Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm wondering since the similar `CodeCache::blob_count()` still uses one of these macros. src/hotspot/share/code/nmethod.cpp line 812: > 810: // By calling this nmethod entry barrier, it plays along and acts > 811: // like any other nmethod found on the stack of a thread (fewer > surprises). > 812: nmethod* nm = as_nmethod_or_null(); Calling as_nmethod_or_null() from within functions in the nmethod class is suspicious. Shouldn't all such usages be removed? (I'm fine with doing that as a separate change) src/hotspot/share/code/nmethod.cpp line 1009: > 1007: // Fill in default values for various flag fields > 1008: void nmethod::init_defaults() { > 1009: { // avoid uninitialized fields, even for short time periods Should these curly braces be removed? src/hotspot/share/code/nmethod.cpp line 2164: > 2162: DTRACE_METHOD_UNLOAD_PROBE(method()); > 2163: > 2164: // If a JVMTI agent has enabled the nmethod Unload event then I think the event is still called CompiledMethodUnload, so this line should probably be reverted. src/hotspot/share/code/nmethod.hpp line 50: > 48: class ScopeDesc; > 49: class CompiledIC; > 50: class MetadataClosure; Maybe merge (and sort) this together with the other forward declarations? src/hotspot/share/code/nmethod.hpp line 905: > 903: > 904: // printing support > 905: void print() const override; Here and a few other places you only use override and not also virtual. This is inconsistent with other functions in this class. (FWIW, I prefer this style with only the override qualifier). src/hotspot/share/code/nmethod.inline.hpp line 60: > 58: // (b) it is a deopt PC > 59: > 60: inline address nmethod::get_deopt_original_pc(const frame* fr) { While reading this PR I wonder if this really belongs in the `nmethod` class or if it would make more sense to have it as a member function in the `frame` class. It is a static function, which uses `fr` sort-of like a `this` pointer. Maybe something to consider for a separate RFE. src/hotspot/share/code/relocInfo.hpp line 39: > 37: class nmethod; > 38: class CodeBlob; > 39: class nmethod; You already have a class nmethod forward declaration above. src/hotspot/share/compiler/compileBroker.cpp line 1379: > 1377: if (osr_bci == InvocationEntryBci) { > 1378: // standard compilation > 1379: nmethod* method_code = method->code(); Isn't the `method_code->is_nmethod()` redundant now? src/hotspot/share/compiler/compileBroker.cpp line 1484: > 1482: // We accept a higher level osr method > 1483: if (osr_bci == InvocationEntryBci) { > 1484: nmethod* code = method->code(); Cast below is redundant. src/hotspot/share/gc/g1/g1HeapRegion.cpp line 339: > 337: > 338: void do_code_blob(CodeBlob* cb) { > 339: nmethod* nm = (cb == nullptr) ? nullptr : cb->as_nmethod_or_null(); After this change I'd like to see if we can change this and similar GC interfaces to work directly against `nmethod` instead of `CodeBlob`. src/hotspot/share/gc/shared/parallelCleaning.cpp line 54: > 52: void CodeCacheUnloadingTask::claim_nmethods(nmethod** claimed_nmethods, > int *num_claimed_nmethods) { > 53: nmethod* first; > 54: NMethodIterator last(NMethodIterator::all_blobs); FWIW, `all_blobs` is slightly confusing name when nmethods are a subset of all "code blobs". We might want to consider renaming this to `NMethodIterator::all` (in a separate RFE). src/hotspot/share/gc/x/xUnload.cpp line 78: > 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour { > 77: public: > 78: virtual bool has_dead_oop(nmethod* method) const { This now takes an `nmethod` argument, but still calls as_nmethod(). I think that should be removed from this, and all similar functions here in the GC code. If you want, I can do that as a follow-up RFE. src/hotspot/share/jvmci/jvmciRuntime.cpp line 271: > 269: > 270: nm = CodeCache::find_nmethod(pc); > 271: assert(nm != nullptr, "this is not a compiled method"); Unrelated to this patch, but might be worth mentioning because I think it would be good to think about this in a follow-up RFE. `CodeCache::find_blob` returns null if it can't find a matching blob, but `CodeCache::find_nmethod` asserts that it did find one. The latter makes the assert redundant, but it also begs to question if `find_blob` and `find_nmethod` realy should be different in this regard? Should we have `find_blob_or_null` and `find_nmethod_or_null`? Alt. `find_blob_not_null` and `find_nmethod_not_null`. src/hotspot/share/prims/whitebox.cpp line 772: > 770: if (_make_not_entrant) { > 771: nmethod* nm = CodeCache::find_nmethod(f->pc()); > 772: assert(nm != nullptr, "sanity check"); This assert is now redundant. src/hotspot/share/prims/whitebox.cpp line 1100: > 1098: // Check code again because compilation may be finished before > Compile_lock is acquired. > 1099: if (bci == InvocationEntryBci) { > 1100: nmethod* code = mh->code(); `as_nmethod_or_null()` below should be redundant. src/hotspot/share/runtime/continuationEntry.hpp line 35: > 33: #include CPU_HEADER(continuationEntry) > 34: > 35: class nmethod; Maybe keep the forward declarations sorted? src/hotspot/share/runtime/continuationEntry.hpp line 59: > 57: public: > 58: static int _return_pc_offset; // friend gen_continuation_enter > 59: static void set_enter_code(nmethod* cm, int interpreted_entry_offset); cm => nm? src/hotspot/share/runtime/frame.cpp line 208: > 206: address frame::raw_pc() const { > 207: if (is_deoptimized_frame()) { > 208: nmethod* nm = cb()->as_nmethod_or_null(); Prexisting: It's weird that this code is using the `_or_null()` version when the code below does not null check the returned value. src/hotspot/share/runtime/vframe.cpp line 75: > 73: if (cb != nullptr) { > 74: if (cb->is_nmethod()) { > 75: nmethod* nm = cb->as_nmethod();; There's two `;`s on this line. ------------- PR Review: https://git.openjdk.org/jdk/pull/18554#pullrequestreview-1977027234 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549873079 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549892107 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549895707 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549917406 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549925499 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549949611 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549954407 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549955841 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549958927 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549968764 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549974539 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549975722 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549977276 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549978007 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549979750 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549983251 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549985971 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549992086 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1549999765 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550002167 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550005072 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550006055 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550013866 PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550025081
