On Mon, 5 Dec 2022 17:45:25 GMT, Doug Simon <[email protected]> wrote:
>> Libgraal is compiled ahead of time and should not need any JVMCI Java code
>> to be dynamically loaded at runtime. Prior to this PR, this is not the case
>> due to:
>>
>> * Code to copy system properties from the HotSpot heap into the libgraal
>> heap. This is in
>> `jdk.vm.ci.services.Services.initializeSavedProperties(byte[])` and
>> `jdk.vm.ci.services.Services.serializeSavedProperties()`. This code should
>> be moved to `java.base/jdk.internal.vm.VMSupport`.
>> * Code to translate exceptions from the HotSpot heap into the libgraal heap
>> and vice versa. This code should be moved from
>> `jdk.internal.vm.ci//jdk.vm.ci.hotspot.TranslatedException` to
>> `java.base/jdk.internal.vm.VMSupport`.
>>
>> This PR makes the above changes. As a result, it's possible to build a JDK
>> image that includes (and uses) libgraal but does not include
>> `jdk.internal.vm.ci` or `jdk.internal.vm.compiler`. This both reduces
>> footprint and better isolates the JAVMCI and the Graal compiler as accessing
>> these components from Java code is now impossible.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> renamed is_module_resolvable to is_module_observable
Sorry Doug not a full review (not familiar enough with code) but a few drive-by
nits.
Thanks.
src/hotspot/share/jvmci/jvmci.cpp line 233:
> 231: Thread* thread = Thread::current_or_null_safe();
> 232: if (thread != nullptr && thread->is_Java_thread()) {
> 233: ResourceMark rm;
You can pass `thread` to the rm constructor to save an internal lookup.
src/hotspot/share/jvmci/jvmci.cpp line 234:
> 232: if (thread != nullptr && thread->is_Java_thread()) {
> 233: ResourceMark rm;
> 234: JavaThreadState state = ((JavaThread*) thread)->thread_state();
Please use `JavaThread::cast(thread)`
src/hotspot/share/jvmci/jvmci.cpp line 241:
> 239: // resolve the j.l.Thread object unless the thread is in
> 240: // one of the states above.
> 241: tty->print("JVMCITrace-%d[%s@" INTPTR_FORMAT "]:%*c", level,
> thread->type_name(), p2i(thread), level, ' ');
I think the current preferred style is to use PTR_FORMAT here.
src/hotspot/share/runtime/arguments.cpp line 1958:
> 1956: AddProperty, UnwriteableProperty, InternalProperty);
> 1957: if (ClassLoader::is_module_observable("jdk.internal.vm.ci")) {
> 1958: if(!create_numbered_module_property("jdk.module.addmods",
> "jdk.internal.vm.ci", addmods_count++)) {
Nit: space after `if` please
src/java.base/share/classes/jdk/internal/vm/TranslatedException.java line 2:
> 1: /*
> 2: * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
If you just moved the file the original copyright year should remain - this is
not new code.
-------------
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.org/jdk/pull/11513
