RFR 8197954 Remove unnecessary intermediary APIs from AppCDS implementation

[Ioi Lam]

On 5/4/18 6:47 PM, Calvin Cheung wrote:
>
>
> 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().
>
Yes, it was used there, but there was no code that would add a 
MODULE_PATH, so this printing code is actually never used.

Thanks
- Ioi

> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>