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/27/2016 7:35 AM, David Holmes wrote:
>
>
> 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?

Yes!

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