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

David

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