Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. Thanks for the detailed clarification! The purpose of this PR is to skip the native call and use ByteOrder. Now I'm convinced that we should not change the initialization order casually, this will break some "future code" and make UnsafeConstants unable to use String. - PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Mon, 28 Jun 2021 03:25:09 GMT, Yi Yang wrote:
>> Prefer using ByteOrder to compute byte order for StringUTF16 to determining
>> byte order by native method StringUTF16.isBigEndian.
>
> Hi Aleksey, do you have a concrete issue/discussion about bootstrapping
> problems? I don't see it because I can build JDK and passes tier1 tests w/o
> problems. But I admit this may cause potential problems. In order to reduce
> potential risks, how about changing the class initialization orders, i.e.
> initialize UUnsafeConstants_klas earlier, which seems reasonable:
>
>
> void Threads::initialize_java_lang_classes(JavaThread* main_thread, TRAPS) {
> TraceTime timer("Initialize java.lang classes", TRACETIME_LOG(Info,
> startuptime));
>
> if (EagerXrunInit && Arguments::init_libraries_at_startup()) {
> create_vm_init_libraries();
> }
>
> +#ifdef ASSERT
> + InstanceKlass *k = vmClasses::UnsafeConstants_klass();
> + assert(k->is_not_initialized(), "UnsafeConstants should not already be
> initialized");
> +#endif
> +
> + // initialize the hardware-specific constants needed by Unsafe
> + initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), CHECK);
> + jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
>
> initialize_class(vmSymbols::java_lang_String(), CHECK);
>
> // Inject CompactStrings value after the static initializers for String ran.
> java_lang_String::set_compact_strings(CompactStrings);
> ...
@kelthuzadx , if you want to discuss initialization order changes please take
this to hotspot-runtime-dev. Simply building okay and running a few tests can
never validate a change in VM initialization order - there are a lot of
subtleties involved, platform differences, affect of different flags, etc etc.
Thanks @adinn for the details on this specific proposal - I agree that we
neither want to make UnsafeConstants unable to use String, nor expose it for
use by String.
Backing up a step what is the motivation for change here? Is there something
wrong with the way String currently does this? Or is it just that it seemed on
the surface that you could skip the native call and "just use Byteorder"?
-
PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. Hi Yi Yang, This is more complex than it seems. The general policy is not to change boot order if at all possible and for good reason. there are many ways it can cause unexpected consequences. Regarding this specific case, UnsafeConstants exists in order to allow class Unsafe to define a few final fields derived from runtime environment/hardware parameters provided as final static *primitive* fields of UnsafeConstants. The relevant fields in UnsafeConstants are injected by the JVM after they have been initialized to dummy values by the class's method. So, the purpose of the class is simply to pre-cache these runtime/hardware values as Java state, avoiding the need for Unsafe to call out to the JVM using native code. Before UnsafeConstants was defined these constants used to be retrieved using native callouts. What you are asking for is to make String use the same pre-cache as Unsafe i.e. to expand the set of classes which depend on UnsafeConstants to include String. While that might work as currently defined it is not clear that it will always continue to work in the future. That's because String is a rather special case, much like a primitive class. Instances of String are created to represent literal terms in the language. Note that these String terms are themselves *constants*. UnsafeConstants is not meant to have any other uses except to cache the runtime-specific/hardware constants used by Unsafe. So, it ought not to depend on other classes. However, it is also important that no other classes depend on it and that includes String. Now imagine someone were to update UnsafeConstants to include an injected String constant -- say, the current setting for LC_LANG, or the processor CPU name or some other legitimate text value derived form the runtime or hardware. Your change would cause a problem because initialization of UnsafeConstants would require String already to be initialized. Of course, neither of those examples is a likely candidate for inclusion in UnsafeConstants but it is hard to say whether other more realistic cases might arise in future. - PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote:
> Prefer using ByteOrder to compute byte order for StringUTF16 to determining
> byte order by native method StringUTF16.isBigEndian.
Hi Aleksey, do you have a concrete issue/discussion about bootstrapping
problems? I don't see it because I can build JDK and passes tier1 tests w/o
problems. But I admit this may cause potential problems. In order to reduce
potential risks, how about changing the class initialization orders, i.e.
initialize UUnsafeConstants_klas earlier, which seems reasonable:
void Threads::initialize_java_lang_classes(JavaThread* main_thread, TRAPS) {
TraceTime timer("Initialize java.lang classes", TRACETIME_LOG(Info,
startuptime));
if (EagerXrunInit && Arguments::init_libraries_at_startup()) {
create_vm_init_libraries();
}
+#ifdef ASSERT
+ InstanceKlass *k = vmClasses::UnsafeConstants_klass();
+ assert(k->is_not_initialized(), "UnsafeConstants should not already be
initialized");
+#endif
+
+ // initialize the hardware-specific constants needed by Unsafe
+ initialize_class(vmSymbols::jdk_internal_misc_UnsafeConstants(), CHECK);
+ jdk_internal_misc_UnsafeConstants::set_unsafe_constants();
initialize_class(vmSymbols::java_lang_String(), CHECK);
// Inject CompactStrings value after the static initializers for String ran.
java_lang_String::set_compact_strings(CompactStrings);
...
-
PR: https://git.openjdk.java.net/jdk/pull/4596
Re: RFR: 8269384: Determine native byte order for StringUTF16 via ByteOrder
On Fri, 25 Jun 2021 13:40:54 GMT, Yi Yang wrote: > Prefer using ByteOrder to compute byte order for StringUTF16 to determining > byte order by native method StringUTF16.isBigEndian. Adding new dependencies in `String` quite probably risks bootstrapping issues. There is a reason why `String` calls into its own native methods for this. - PR: https://git.openjdk.java.net/jdk/pull/4596
