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 7:57 PM, Lois Foltan wrote:
>
> 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!

Yes _but_ the optimization is only used if we hit a GC safepoint, which 
you already agreed was very unlikely during the initialization phase.

David
-----


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