RFR (M): JDK-8159262: Walking PackageEntry Export and ModuleEntry Reads Must Occur Only When Neccessary And Wait Until ClassLoader's Aliveness Determined

[Lois Foltan]

On 6/24/2016 11:03 PM, David Holmes wrote:
> On 25/06/2016 8:04 AM, Lois Foltan wrote:
>>
>> On 6/24/2016 5:51 PM, David Holmes wrote:
>>>
>>>
>>> On 25/06/2016 3:37 AM, Lois Foltan wrote:
>>>>
>>>> On 6/23/2016 5:02 PM, David Holmes wrote:
>>>>> On 23/06/2016 11:53 PM, Lois Foltan wrote:
>>>>>>
>>>>>> On 6/23/2016 7:37 AM, David Holmes wrote:
>>>>>>> On 23/06/2016 9:25 PM, Alan Bateman wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 23/06/2016 12:05, David Holmes wrote:
>>>>>>>>>
>>>>>>>>> You said:
>>>>>>>>>
>>>>>>>>> "Module system initialization will initialize the built-in class
>>>>>>>>> loaders, including the application class loader. The platform or
>>>>>>>>> application class loaders won't load any classes in this phase of
>>>>>>>>> course but they will be initialized (with the modules that they
>>>>>>>>> might
>>>>>>>>> later load classes from). "
>>>>>>>>>
>>>>>>>>> What does that last part mean: "with the modules they might later
>>>>>>>>> load
>>>>>>>>> classes from"? How have modules become bound to a classloader 
>>>>>>>>> that
>>>>>>>>> may
>>>>>>>>> not even get instantiated? If that class loader is never
>>>>>>>>> instantiated
>>>>>>>>> then why do we need a runtime test that checks for the type of 
>>>>>>>>> the
>>>>>>>>> actual system class loader?
>>>>>>>> The graph of modules that are defined to the runtime during 
>>>>>>>> startup
>>>>>>>> are
>>>>>>>> we call the "boot layer". They are statically mapped to the 
>>>>>>>> built-in
>>>>>>>> class loaders where built-in means the boot, platform or 
>>>>>>>> application
>>>>>>>> class loaders. There may be a custom system class loader that 
>>>>>>>> comes
>>>>>>>> into
>>>>>>>> existence later during the startup but it's just not 
>>>>>>>> interesting to
>>>>>>>> the
>>>>>>>> discussion here (except to know that it might be different to the
>>>>>>>> application class loader in some niche configuration).
>>>>>>>
>>>>>>> But you skipped the key part - why do we care about the built-in
>>>>>>> loaders. In this code:
>>>>>>>
>>>>>>> +// If the module's loader, that a read edge is being established
>>>>>>> to, is
>>>>>>> +// not the same loader as this module's and is not one of the 3
>>>>>>> builtin
>>>>>>> +// class loaders, then this module's reads list must be walked 
>>>>>>> at GC
>>>>>>> +// safepoint. Modules have the same life cycle as their defining
>>>>>>> class
>>>>>>> +// loaders and should be removed if dead.
>>>>>>> +void ModuleEntry::set_read_walk_required(ClassLoaderData*
>>>>>>> m_loader_data) {
>>>>>>> +  assert_locked_or_safepoint(Module_lock);
>>>>>>> +  if (!_must_walk_reads &&
>>>>>>> +      loader_data() != m_loader_data &&
>>>>>>> + !m_loader_data->is_builtin_class_loader_data()) {
>>>>>>> +    _must_walk_reads = true;
>>>>>>> +    if (log_is_enabled(Trace, modules)) {
>>>>>>> +      ResourceMark rm;
>>>>>>> + log_trace(modules)("ModuleEntry::set_read_walk_required():
>>>>>>> module %s reads list must be walked",
>>>>>>> +                         (name() != NULL) ? 
>>>>>>> name()->as_C_string() :
>>>>>>> UNNAMED_MODULE);
>>>>>>> +    }
>>>>>>> +  }
>>>>>>> +}
>>>>>>>
>>>>>>> what is "is_builtin_class_loader_data" really asking? Is it just 
>>>>>>> that
>>>>>>> this is a loader whose lifetime is matched to that of the VM? If so
>>>>>>> then it is asking the wrong question with respect to the system
>>>>>>> loader. If not, then what is it about the built-in system loader 
>>>>>>> type
>>>>>>> that makes it different to some custom system loader?
>>>>>>
>>>>>> Hi David,
>>>>>>
>>>>>> The 3 built-in loaders will never be GC'ed, neither will a custom
>>>>>> system
>>>>>> classloader if there is one configured.  Thus the JVM can make
>>>>>> reliable
>>>>>> assumptions based on modules defined to those 3 loaders.  So if for
>>>>>> example, a module's reads list only has established read edges to
>>>>>> modules that are defined to any of the 3 built-in loaders, the 
>>>>>> JVM can
>>>>>> rely on the fact that these loaders will never die and thus their
>>>>>> modules will never die.  So at a GC safepoint, that module's reads
>>>>>> list
>>>>>> will not have to be walked looking for dead modules that should be
>>>>>> removed.
>>>>>
>>>>> <redface> My apologies to Alan and you as I didn't read this code you
>>>>> updated properly
>>>>
>>>> No worries!
>>>>
>>>>>
>>>>> +bool SystemDictionary::is_system_class_loader(Handle class_loader) {
>>>>> +  if (class_loader.is_null()) {
>>>>> +    return false;
>>>>> +  }
>>>>> +  return (class_loader->klass() ==
>>>>> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass() 
>>>>>
>>>>>
>>>>> ||
>>>>> +          class_loader() == _java_system_loader);
>>>>> +}
>>>>>
>>>>> </redface>
>>>>>
>>>>> Though I'm still unclear why it needs to have this form rather than
>>>>> just a check for:
>>>>>
>>>>> class_loader() == _java_system_loader
>>>>>
>>>>> Even if _java_system_loader may not be initialized if this is called
>>>>> very early during the init phase (seems unlikely but possible), then
>>>>> all that will happen is we do an unnecessary walk of the reads list.
>>>>> That seems better than having to do the klass check each time this is
>>>>> called.
>>>>
>>>> But it is called very and often in module initialization since the
>>>> module graph (set of root modules and the transitive closure of their
>>>> dependencies with respect to the set of observable modules), is
>>>> communicated to the JVM during init phase 2.  For example, a simple
>>>> "java -version" defines several modules, establishes read edges 
>>>> between
>>>> modules and numerous qualified exports.  More specific details can be
>>>> obtained via -Xlog:modules=debug.
>>>
>>> You previously indicated this was needed to avoid walking the read
>>> edges during a GC safepoint. There should not be many (if any) GC's
>>> during VM initialization. ??
>>
>> Correct, there should not be many GC's during VM initialization.
>> However, the module graph is intact for the life of the application in
>> order for the JVM to correctly adhere to the new module rules with
>> regards to type accessibility/access checking.  And it can be added to
>> dynamically.  So it subject to being walked during any GC post VM
>> initialization.
>
> Okay, but that still leaves me wondering why we can't just check:
>
> class_loader() == _java_system_loader
>
> as the check for:
>
> class_loader->klass() == 
> SystemDictionary::jdk_internal_loader_ClassLoaders_AppClassLoader_klass()
>
> seems to be an unnecessary optimization only useful during 
> initialization ??

The modules defined, the read edges and the exported packages 
established during module system initialization (init phase 2) are the 
ones who can most benefit by this optimization since they are largely 
the system modules defined to the 3 builtin loaders!  During module 
system initialization, for example, the module jdk.jconsole is defined 
to the builtin application class loader and establishes several read 
edges some of which are the following:

[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.logging
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.datatransfer
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module jdk.attach
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.xml
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.rmi
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module jdk.jvmstat
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.desktop
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.base
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module java.management
[0.332s][debug][modules] add_reads_module(): Adding read from module 
jdk.jconsole to module jdk.management

All the modules it establishes read edges to are modules that are 
defined to the 3 builtin loaders.  It would be beneficial to never have 
to walk the jdk.jconsole's ModuleEntry reads list looking for dead 
modules that should be removed because in most scenarios there aren't 
going to be any.

>
> David
>
>>>
>>> David
>>>
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>>> Lois
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> (PS. Really calling it a night now :) )
>>>>>>>
>>>>>>>> -Alan
>>>>>>
>>>>
>>