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/26/2016 7:32 PM, David Holmes wrote:
> 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.

But these moduleEntry(s) stay alive for the life of the application so 
are subject to being walked whenever there is a GC safepoint, not just 
during initialization.
Lois

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