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

David

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