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

[David Holmes]

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

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

Thanks,
David

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