RFR 8197954 Remove unnecessary intermediary APIs from AppCDS implementation

[Calvin Cheung]

On 5/4/18, 2:09 PM, Ioi Lam wrote:
> On 5/4/18 12:54 PM, Jiangli Zhou wrote:
>> Looks good. My only question is why not using CLASS_PATH, but I think 
>> probably your concern is naming conflict. Using ‘APP_PATH’ is ok.
>>
> One reason is we have many places that refer to "app" vs "boot" paths, 
> so if we just say "CLASS_PATH" it is not consist with the rest of the 
> AppCDS code.
>
> By the way, the CLASSPATH has traditionally been called many different 
> names -- the classpath, the app classpath, the system classpath. Maybe 
> we should clean all that up in a separate RFE.
>
> Also, I found out that SharedPathsMiscInfo::MODULE_PATH is not used 
> anywhere, so I will delete it.
Do you mean the MODULE_PATH in the enum?
I saw it's being referenced in SharedPathsMiscInfo::print_path().

Looks good otherwise.

thanks,
Calvin
>
> Thanks
> - Ioi
>
>> Thanks,
>>
>> Jiangli
>>
>>> On May 4, 2018, at 11:05 AM, Ioi Lam <ioi.lam at oracle.com 
>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>
>>> Hi Jiangli,
>>>
>>> Thank you so much for the review. I've made changes according to 
>>> your and Calvin's suggestions:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk11/8197954-remove-unnecessary-appcds-api.v02.diff/ 
>>>
>>>
>>>
>>> On 5/3/18 12:43 PM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>>
>>>> Thank you for removing the _ext files and merging the code 
>>>> together. It’s very helpful.
>>>>
>>>> - sharedPathsMiscInfo.cpp
>>>>
>>>> 191 // FIXME: why is there no MODULE check?
>>>>
>>>> The path check for archived module classes is handled differently 
>>>> than the classes from the -cp or -Xbootclasspath/a because we can 
>>>> quickly determine if an archived module class’ origin matches with 
>>>> the runtime module source path. That’s why we don’t  do string 
>>>> comparison of the dump time module path and the runtime module path 
>>>> in SharedPathsMiscInfo::check(), which is too restrictive and 
>>>> limited. Could you please replace the FIXME comment?
>>>>
>>> Fixed. I replaced the FIXME with your comments.
>>>> - sharedPathsMiscInfo.hpp
>>>>
>>>> 131 const char* type_name(int type) {
>>>>   132     switch (type) {
>>>>   133     case BOOT:      return "BOOT";
>>>>   134     case NON_EXIST: return "NON_EXIST";
>>>> 135 case APP: return "APP";
>>>> 136 case MODULE: return "MODULE";
>>>>
>>>> The types are not well-defined. That’s not introduced by your 
>>>> change. The ‘APP’ type became inaccurate when we added the ‘MODULE’ 
>>>> type. It's noticeable now when types are merged together. To avoid 
>>>> overlapping, ‘APP' can be changed to ‘CLASS_PATH’ or ‘CP’. How 
>>>> about renaming the types to following:
>>>>
>>>>     BOOT_PATH
>>>>     CLASS_PATH
>>>>     MODULE_PATH
>>>>     NON_EXIST
>>>>
>>>>
>>>> As your current change does not involve larger scale restructuring, 
>>>> it’s okay if we follow up with a separate RFE for the type renaming.
>>>>
>>> I changed the names it as you suggested.
>>>
>>>> - systemDictionaryShared.cpp
>>>>
>>>> 490 // [c] At this point, if the named class was loaded by the
>>>> 491 // AppClassLoader during archive dump time, we know that it 
>>>> must be
>>>> 492 // loaded by the AppClassLoader during run time, and will not 
>>>> be loaded
>>>> 493 // by a delegated class loader.
>>>> The above only applies to the application classes loaded from -cp, 
>>>> but not modules. An archived module class may not be used at 
>>>> runtime if the module has a different source path at runtime. Those 
>>>> archived module classes are filtered out by the runtime shared 
>>>> class ‘visibility’ check.
>>>>
>>>> 493 // by a delegated class loader. This is true because we have 
>>>> checked the
>>>> 494 // CLASSPATH and module path to ensure compatibility between 
>>>> dump time and
>>>> 495 // run time.
>>>>
>>>> Please remove ‘module path’ from above comment. We don’t do string 
>>>> comparison of the runtime module path and dump time module path as 
>>>> CLASSPATH.
>>>>
>>>> The comment [C] probably should be split into two part, one for 
>>>> application classes from -cp and the other part for application 
>>>> module classes.
>>>>
>>>> 508 // An alternative is to modify the Java code of 
>>>> BuiltinClassLoader.loadClassOrNull().
>>>> Should above be removed? Otherwise, more details are needed.
>>>>
>>> I've updated the comments. Because there's already comments around 
>>> is_shared_class_visible_for_classloader(), I just put a reference to 
>>> that, so we can avoid duplication. What do you think?
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>> On May 1, 2018, at 10:17 PM, Ioi Lam <ioi.lam at oracle.com 
>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8197954
>>>>> http://cr.openjdk.java.net/~iklam/jdk11/8197954-remove-unnecessary-appcds-api.v01/ 
>>>>>
>>>>>
>>>>> Summary:
>>>>>
>>>>> Before AppCDS was open sourced, we had a few "Ext" classes in 
>>>>> sharedClassUtil.hpp
>>>>> that abstracted away the CDS operations related to the non-boot 
>>>>> class loaders.
>>>>> This API made it possible to build the Oracle JDK with AppCDS 
>>>>> included, or build
>>>>> the Open JDK with AppCDS removed.
>>>>>
>>>>> With the open sourcing of AppCDS, this abstraction layer is no 
>>>>> longer necessary. I
>>>>> have moved the contents of the "Ext" classes into their proper 
>>>>> locations and removed
>>>>> the sharedClassUtil.hpp/cpp files.
>>>>>
>>>>> Most of the changes are just moving things around. There shouldn't 
>>>>> be behavioral
>>>>> changes.
>>>>>
>>>>> The files classLoaderExt.hpp/cpp still exists -- it encapsulates 
>>>>> the classpath
>>>>> management code and is not strictly unnecessary. Some clean up may 
>>>>> be needed there, but
>>>>> I'll probably do that in a separate RFE.
>>>>>
>>>>> Testing with hotspot tiers1~4.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>
>>>
>>
>