RFR: 8264538: Rename SystemDictionary::parse_stream

[Coleen Phillimore-3]

This function is used to call the classfile parser for hidden or anonymous classes, and for use with jvmti RedefineClasses. The latter only calls KlassFactory::create_from_stream and skips the rest of the code in SystemDictionary::parse_stream.

Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
resolve_from_stream -> resolve_class_from_stream
and have SystemDictionary::resolve_from_stream() call the right version depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass protection domain via. ClassLoadInfo.

So the external API is resolve_from_stream.

Tested with tier1 on 4 Oracle supported platforms.

-------------

Commit messages:
 - Rename parse_stream

Changes: https://git.openjdk.java.net/jdk/pull/3289/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3289&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264538
  Stats: 132 lines in 7 files changed: 37 ins; 23 del; 72 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3289.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3289/head:pull/3289

PR: https://git.openjdk.java.net/jdk/pull/3289

[Lois Foltan-3]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This function is used to call the classfile parser for hidden or anonymous classes, and for use with jvmti RedefineClasses. The latter only calls KlassFactory::create_from_stream and skips the rest of the code in SystemDictionary::parse_stream.
>>
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass protection domain via. ClassLoadInfo.
>>
>> So the external API is resolve_from_stream.
>>
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   fifix comment

Nice clean up Coleen.  One minor comment.

Thanks,
Lois

src/hotspot/share/prims/jvm.cpp line 950:

> 948:   InstanceKlass* ik = NULL;
> 949:   if (!is_hidden) {
> 950:     ClassLoadInfo cl_info(protection_domain);

Minor comment, you could pull the creation of ClassLoadInfo out of this if statement since both the the if and the else sections create a ClassLoadInfo with pretty much the same information.

-------------

Marked as reviewed by lfoltan (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3289

[Harold Seigel-2]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This function is used to call the classfile parser for hidden or anonymous classes, and for use with jvmti RedefineClasses. The latter only calls KlassFactory::create_from_stream and skips the rest of the code in SystemDictionary::parse_stream.
>>
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass protection domain via. ClassLoadInfo.
>>
>> So the external API is resolve_from_stream.
>>
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   fifix comment

src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1395:

> 1393:                                                       cl_info,
> 1394:                                                       THREAD);
> 1395:

Could you add a comment above line 1390 saying you can't call resolve_class_from_stream() here because the resulting class should not go in the system dictionary?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3289

[Harold Seigel-2]

On Wed, 31 Mar 2021 19:57:57 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This function is used to call the classfile parser for hidden or anonymous classes, and for use with jvmti RedefineClasses. The latter only calls KlassFactory::create_from_stream and skips the rest of the code in SystemDictionary::parse_stream.
>>
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass protection domain via. ClassLoadInfo.
>>
>> So the external API is resolve_from_stream.
>>
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   fifix comment

src/hotspot/share/prims/jvmtiRedefineClasses.hpp line 305:

> 303: // - How do we serialize the RedefineClasses() API without deadlocking?
> 304: //
> 305: // - KlassFactory::create_from_stream() was called with a NULL protection

Maybe delete the comment that goes from lines 305 - 309 ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3289

[Harold Seigel-2]

On Wed, 31 Mar 2021 21:41:39 GMT, Coleen Phillimore <[hidden email]> wrote:

>> This function is used to call the classfile parser for hidden or anonymous classes, and for use with jvmti RedefineClasses. The latter only calls KlassFactory::create_from_stream and skips the rest of the code in SystemDictionary::parse_stream.
>>
>> Renamed SystemDictionary::parse_stream -> resolve_hidden_class_from_stream
>> resolve_from_stream -> resolve_class_from_stream
>> and have SystemDictionary::resolve_from_stream() call the right version depending on ClassLoadInfo flags.  Callers of resolve_from_stream now pass protection domain via. ClassLoadInfo.
>>
>> So the external API is resolve_from_stream.
>>
>> Tested with tier1 on 4 Oracle supported platforms.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add and remove comments.

The changes look good!
Thanks, Harold

-------------

Marked as reviewed by hseigel (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3289

[Lois Foltan-3]

On Wed, 31 Mar 2021 21:22:39 GMT, Coleen Phillimore <[hidden email]> wrote:

>> src/hotspot/share/prims/jvm.cpp line 950:
>>
>>> 948:   InstanceKlass* ik = NULL;
>>> 949:   if (!is_hidden) {
>>> 950:     ClassLoadInfo cl_info(protection_domain);
>>
>> Minor comment, you could pull the creation of ClassLoadInfo out of this if statement since both the the if and the else sections create a ClassLoadInfo with pretty much the same information.
>
> That other ClassLoadInfo cl_info(protection_domain) you see is from another function, so I can't pull it out.
> The other side of the 'if' statement creates a ClassLoadInfo with all the hidden class goodies.

In jvm_lookup_define_class there are 2 ClassLoadInfo cl_info constructions on line #950 and line #961 that could be pull out of the if statement.  Again comment is minor and I am ok with how you decide to go forward with it.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3289

[Coleen Phillimore-2]

On 4/1/21 9:48 AM, Lois Foltan wrote:

> On Wed, 31 Mar 2021 21:22:39 GMT, Coleen Phillimore <[hidden email]> wrote:
>
>>> src/hotspot/share/prims/jvm.cpp line 950:
>>>
>>>> 948:   InstanceKlass* ik = NULL;
>>>> 949:   if (!is_hidden) {
>>>> 950:     ClassLoadInfo cl_info(protection_domain);
>>> Minor comment, you could pull the creation of ClassLoadInfo out of this if statement since both the the if and the else sections create a ClassLoadInfo with pretty much the same information.
>> That other ClassLoadInfo cl_info(protection_domain) you see is from another function, so I can't pull it out.
>> The other side of the 'if' statement creates a ClassLoadInfo with all the hidden class goodies.
> In jvm_lookup_define_class there are 2 ClassLoadInfo cl_info constructions on line #950 and line #961 that could be pull out of the if statement.  Again comment is minor and I am ok with how you decide to go forward with it.

Sorry I didn't see this comment in the pull request, and thanks for
explaining that the ClassLoadInfo from the 'else' clause could have been
pulled out of the if statement.  I was thinking the one in the 'if'
clause.  We can change it if we're in the area next time.

Thanks,
Coleen

>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/3289