RFR 8197954 Remove unnecessary intermediary APIs from AppCDS implementation

[Ioi Lam]

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