RFR 8197954 Remove unnecessary intermediary APIs from AppCDS implementation

[Ioi Lam]

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.

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