Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam 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 > > LGTM. Just a small nit. > @iklam > I thought the fingerprint code was also used by CDS. CDS actually uses a different mechanism (CRC of the classfile bytes) to validate that a class has not changed between archive dumping time and runtime. See https://github.com/openjdk/jdk/blob/5784f6b7f74d0b8081ac107fea172539d57d6020/src/hotspot/share/classfile/systemDictionaryShared.cpp#L1126-L1130 - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam 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 > > LGTM. Just a small nit. @iklam I thought the fingerprint code was also used by CDS. - 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 reviewed the module-loader-map and non-hotspot non-AOT tests. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:35 GMT, Ioi Lam 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
>
> src/hotspot/share/oops/methodCounters.cpp line 77:
>
>> 75: }
>> 76:
>> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
>
> MethodCounters::metaspace_pointers_do can be removed altogether (also need to
> remove the declaration in methodCounter.hpp).
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:34:58 GMT, Igor Veresov 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
>
> src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 1184:
>
>> 1182: }
>> 1183: } else if
>> (jvmci_env()->isa_HotSpotMetaspaceConstantImpl(constant)) {
>> 1184: if (!_immutable_pic_compilation) {
>
> All _immutable_pic_compilation mentions can be removed as well. It is true
> only for AOT compiles produced by Graal. It's never going to be used without
> AOT.
Done. I removed _immutable_pic_compilation in JVMCI in Hotspot.
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:30:41 GMT, Igor Veresov 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 > > src/hotspot/share/oops/instanceKlass.hpp line 257: > >> 255: _misc_declares_nonstatic_concrete_methods = 1 << 6, // directly >> declares non-static, concrete methods >> 256: _misc_has_been_redefined = 1 << 7, // class has >> been redefined >> 257: _unused = 1 << 8, // > > Any particular reason we don't want to remove this gap? Less changes. We don't get any benefits from removing it. It is opposite - if we need a new value we will use it without changing following values again. - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 08:32:59 GMT, Aleksey Shipilev 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
>
> src/hotspot/share/code/compiledIC.cpp line 715:
>
>> 713: tty->print("interpreted");
>> 714: } else {
>> 715: tty->print("unknown");
>
> We can probably split this cleanup into the minor issue, your call. The
> benefit of separate issue: backportability.
Reverted and filed https://bugs.openjdk.java.net/browse/JDK-8265013
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 08:29:21 GMT, Aleksey Shipilev 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 > > src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 73: > >> 71: >> 72: #if INCLUDE_JVMCI >> 73: #define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS >> (EnableJVMCI) > > Minor nit: can probably drop parentheses here. done - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 03:03:33 GMT, David Holmes 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
>
> src/hotspot/share/memory/heap.hpp line 174:
>
>> 172: bool contains(const void* p) const { return low() <= p &&
>> p < high(); }
>> 173: bool contains_blob(const CodeBlob* blob) const {
>> 174: const void* start = (void*)blob;
>
> start seems redundant now
Done:
bool contains_blob(const CodeBlob* blob) const {
-const void* start = (void*)blob;
-return contains(start);
+return contains((void*)blob);
}
-
PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 02:44:23 GMT, David Holmes 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 > > src/hotspot/cpu/x86/compiledIC_x86.cpp line 134: > >> 132: #ifdef ASSERT >> 133: CodeBlob *cb = CodeCache::find_blob_unsafe((address) _call); >> 134: assert(cb, "sanity"); > > Nit: implied boolean - use "cb != NULL" done - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 17:09:58 GMT, Vladimir Kozlov wrote: >> Hi Vladimir, >> >> This looks good to me - I went through all the files. >> >> It was nice to see how nicely compartmentalized the AOT feature was - that >> made checking the changes quite simple. The one exception was the >> fingerprinting code, which for some reason was AOT-only but not marked that >> way, so I'm not sure what the story is there. ?? >> >> I was also surprised to see some of the flags were not marked experimental, >> so there will need to a CSR request to remove those without going through >> the normal deprecation process. >> >> Thanks, >> David > >> Hi Vladimir, >> >> This looks good to me - I went through all the files. >> >> It was nice to see how nicely compartmentalized the AOT feature was - that >> made checking the changes quite simple. The one exception was the >> fingerprinting code, which for some reason was AOT-only but not marked that >> way, so I'm not sure what the story is there. ?? >> >> I was also surprised to see some of the flags were not marked experimental, >> so there will need to a CSR request to remove those without going through >> the normal deprecation process. >> >> Thanks, >> David > > Thank you, David. > We thought that we could use fingerprint code for other cases that is why we > did not put it under `#if INCLUDE_AOT`. > I filed CSR for AOT product flags removal: > https://bugs.openjdk.java.net/browse/JDK-8265000 Thank you, Igor Ignatyev, Igor Veresov, Ioi, Aleksey and Andrew for reviews. I will update changes based on your comments. - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 04:32:14 GMT, David Holmes wrote: > Hi Vladimir, > > This looks good to me - I went through all the files. > > It was nice to see how nicely compartmentalized the AOT feature was - that > made checking the changes quite simple. The one exception was the > fingerprinting code, which for some reason was AOT-only but not marked that > way, so I'm not sure what the story is there. ?? > > I was also surprised to see some of the flags were not marked experimental, > so there will need to a CSR request to remove those without going through the > normal deprecation process. > > Thanks, > David Thank you, David. We thought that we could use fingerprint code for other cases that is why we did not put it under `#if INCLUDE_AOT`. I filed CSR for AOT product flags removal: https://bugs.openjdk.java.net/browse/JDK-8265000 - 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
LGTM. Just a small nit.
src/hotspot/share/oops/methodCounters.cpp line 77:
> 75: }
> 76:
> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
MethodCounters::metaspace_pointers_do can be removed altogether (also need to
remove the declaration in methodCounter.hpp).
-
Marked as reviewed by iklam (Reviewer).
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
src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 1184:
> 1182: }
> 1183: } else if (jvmci_env()->isa_HotSpotMetaspaceConstantImpl(constant))
> {
> 1184: if (!_immutable_pic_compilation) {
All _immutable_pic_compilation mentions can be removed as well. It is true only
for AOT compiles produced by Graal. It's never going to be used without AOT.
src/hotspot/share/oops/instanceKlass.hpp line 257:
> 255: _misc_declares_nonstatic_concrete_methods = 1 << 6, // directly
> declares non-static, concrete methods
> 256: _misc_has_been_redefined = 1 << 7, // class has
> been redefined
> 257: _unused = 1 << 8, //
Any particular reason we don't want to remove this gap?
-
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
Shenandoah parts look good. I have a few minor comments after reading the rest.
src/hotspot/cpu/x86/globalDefinitions_x86.hpp line 73:
> 71:
> 72: #if INCLUDE_JVMCI
> 73: #define COMPRESSED_CLASS_POINTERS_DEPENDS_ON_COMPRESSED_OOPS (EnableJVMCI)
Minor nit: can probably drop parentheses here.
src/hotspot/share/code/compiledIC.cpp line 715:
> 713: tty->print("interpreted");
> 714: } else {
> 715: tty->print("unknown");
We can probably split this cleanup into the minor issue, your call. The benefit
of separate issue: backportability.
-
Marked as reviewed by shade (Reviewer).
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 AArch64 looks fine. - Marked as reviewed by aph (Reviewer). 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
Hi Vladimir,
This looks good to me - I went through all the files.
It was nice to see how nicely compartmentalized the AOT feature was - that made
checking the changes quite simple. The one exception was the fingerprinting
code, which for some reason was AOT-only but not marked that way, so I'm not
sure what the story is there. ??
I was also surprised to see some of the flags were not marked experimental, so
there will need to a CSR request to remove those without going through the
normal deprecation process.
Thanks,
David
src/hotspot/cpu/x86/compiledIC_x86.cpp line 134:
> 132: #ifdef ASSERT
> 133: CodeBlob *cb = CodeCache::find_blob_unsafe((address) _call);
> 134: assert(cb, "sanity");
Nit: implied boolean - use "cb != NULL"
src/hotspot/share/memory/heap.hpp line 174:
> 172: bool contains(const void* p) const { return low() <= p &&
> p < high(); }
> 173: bool contains_blob(const CodeBlob* blob) const {
> 174: const void* start = (void*)blob;
start seems redundant now
-
Marked as reviewed by dholmes (Reviewer).
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 20:59:59 GMT, Coleen Phillimore wrote:
> 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.
https://bugs.openjdk.java.net/browse/JDK-8264941
-
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 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 Test changes look good to me. - Marked as reviewed by iignatyev (Reviewer). 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 20:00:30 GMT, Vladimir Kozlov wrote:
>> GC changes look good.
>
>> 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.
>
> Thank you, Coleen, for review. I will wait other comments and will remove
> this code.
Thank you, Erik and Stefan.
-
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 GC changes look good. - Marked as reviewed by stefank (Reviewer). 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 19:58:11 GMT, Stefan Karlsson 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
>
> GC changes look good.
> 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.
Thank you, Coleen, for review. I will wait other comments and will remove this
code.
-
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 Build changes look good. - Marked as reviewed by erikj (Reviewer). 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 [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3398/files - new: https://git.openjdk.java.net/jdk/pull/3398/files/3dbc6210..6cce0f6c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3398=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3398=02-03 Stats: 39 lines in 3 files changed: 0 ins; 36 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3398.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3398/head:pull/3398 PR: https://git.openjdk.java.net/jdk/pull/3398
