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 27/06/2016 9:03 PM, Lois Foltan wrote:
>
> 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.

Ah so I think I misunderstood. I assumed this test was used during GC to 
decide whether to walk the read edges of a module. It seems you are 
using it to "tag" a module with whether the reads edges need to be 
walked - is that right?

Thanks,
David


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