RFR 8197954 Remove unnecessary intermediary APIs from AppCDS implementation

[Jiangli Zhou]

> On May 4, 2018, at 2:09 PM, Ioi Lam <ioi.lam at oracle.com> 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.

Ok.

Thanks,
Jiangli

> 
> 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/ <http://cr.openjdk.java.net/%7Eiklam/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 <https://bugs.openjdk.java.net/browse/JDK-8197954>
>>>>> http://cr.openjdk.java.net/~iklam/jdk11/8197954-remove-unnecessary-appcds-api.v01/ <http://cr.openjdk.java.net/%7Eiklam/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
>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>