(11) RFR (S) JDK-8205509: assert(_name_and_id != 0LL) failed: encountered a class loader null name and id

[Lois Foltan]

On 6/24/2018 1:02 PM, Thomas Stüfe wrote:

> Hi Lois,
>
> thanks for the explanations!
>
> If you just change the comment, I do not need another webrev.

Done, thanks again for the review!
Lois

>
> Best Regards, Thomas
>
> On Sun, Jun 24, 2018 at 2:12 PM, Lois Foltan <lois.foltan at oracle.com> wrote:
>> On 6/23/2018 3:03 AM, Thomas Stüfe wrote:
>>
>>> Hi Lois,
>>>
>>> This looks good, thanks for fixing.
>> Thanks for the review Thomas!
>>>
>>> I guess that means the return value of CLD::name() and
>>> CLD::name_and_id() may "fluctuate" under rare conditions, yes?
>> Yes.
>>
>>> You could make _class_loader_klass const and initialize it in the
>>> CLD::CLD initializer list.
>> I could except that the bootstrap class loader case has to be checked before
>> setting _class_loader_klass to the oop's klass().
>>
>>> I try to understand why we cannot initialize _name and _name_and_id in
>>> the constructor too. In ClassLoaderDataGraph::add() I see we do
>>> initialize them explicitly after adding them to the CLDG because
>>> "because adding the Symbol for the name might safepoint." (btw I like
>>> safepoint as a verb :-). But where, in
>>> ClassLoaderData::initialize_name_and_klass(), could we possibly
>>> safepoint? Sorry if the question is stupid. The symbol lives in C-heap
>>> (resp. arena, which in the end is the same), so allocating a Symbol
>>> should not safepoint?
>> The issue is within ClassLoaderData's constructor and I'm borrowing Coleen's
>> description from a previous email about this.
>>
>> It's just kind of an inconvenience and at a sketchy time.  If you look at
>> the constructor ClassLoaderData::ClassLoaderData, you'd have to have the
>> safepoint before lines:
>>
>>    if (!h_class_loader.is_null()) {
>>      _class_loader = _handles.add(h_class_loader());
>>    }
>>
>> Because the ClassLoaderData hasn't been added to the graph yet, and if
>> there's some GC, this _handles[] cell will not get followed. We've had many
>> problems like this in the past because we had more oops that were in the
>> ClassLoaderData.  There are just some hidden dependencies on what the code
>> does at this point and having the constructor do miminal things helps.
>>
>>> If the comment in CLDG::add() is still correct, could you modify it to
>>> "adding the Symbols for name and name_and_id"?
>> Will do.
>> Thanks,
>> Lois
>>
>>
>>> Thank you, Thomas
>>>
>>> On Sat, Jun 23, 2018 at 1:39 AM, Lois Foltan <lois.foltan at oracle.com>
>>> wrote:
>>>> On 6/22/2018 7:28 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>> Lois,
>>>>> This looks good!
>>>>>
>>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509/webrev/src/hotspot/share/classfile/classLoaderData.cpp.udiff.html
>>>>>
>>>>>      if (!h_class_loader.is_null()) {
>>>>>        _class_loader = _handles.add(h_class_loader());
>>>>>      }
>>>>>
>>>>> + if (!h_class_loader.is_null()) {
>>>>> + _class_loader_klass = h_class_loader->klass();
>>>>> + }
>>>>> +
>>>>>
>>>>> Can you put the _class_loader_klass under the if statement above it?
>>>>>
>>>>> + } else {
>>>>> + return _class_loader_klass->external_name();
>>>>>
>>>>> Can you add short comment:  // May be called in a race before
>>>>> _name_and_id
>>>>> is initialized.
>>>>>
>>>>> Unfortunately, there isn't a good way to assert this (other than maybe a
>>>>> flag which would serve the same purpose as a null _name_and_id), and
>>>>> this
>>>>> code path is sensitive to safepoints because the link from class_loader
>>>>> to
>>>>> ClassLoaderData and the addition of ClassLoaderData to the CLDG needs to
>>>>> appear atomic.
>>>> Hi Coleen,
>>>>
>>>> Thanks for the review, good comments.  They all have been addressed and
>>>> more
>>>> testing is in progress.  See updated webrev at:
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509.1/webrev/
>>>>
>>>> Lois
>>>>
>>>>
>>>>> Thanks for the speedy fix.
>>>>> Coleen
>>>>>
>>>>> On 6/22/18 7:00 PM, Lois Foltan wrote:
>>>>>> Please review the following change to address a hs-tier5 test failure.
>>>>>> The recent change for JDK-8202605 aggravated an existing race condition
>>>>>> within ClassLoaderDataGraph::add() where a given class loader's
>>>>>> ClassLoaderData structure is added to the ClassLoaderDataGraph prior to
>>>>>> full
>>>>>> initialization of the CLD's _class_loader_klass, _name and _name_and_id
>>>>>> fields within ClassLoaderData::initialize_name_and_klass.  To ensure
>>>>>> that
>>>>>> methods loader_name() and loader_name_and_id() always provide a string
>>>>>> for
>>>>>> the class loader name, the fix involved moving the field,
>>>>>> _class_loader_klass, to the ClassLoaderData's constructor where if
>>>>>> _name
>>>>>> and/or _name_and_id fields are null, a reasonable _class_loader_klass's
>>>>>> external_name() can be provided.
>>>>>>
>>>>>> open webrev http://cr.openjdk.java.net/~lfoltan/bug_jdk8205509/webrev/
>>>>>> bug link at https://bugs.openjdk.java.net/browse/JDK-8205509
>>>>>>
>>>>>> Testing: hs-tier(1-2), jdk-tier(1-3) complete
>>>>>>                  hs-tier3-5 in progress
>>>>>>                  --test-repeat 50 ran for tier1 platforms of test
>>>>>> jdk/java/lang/instrument/DaemonThread/TestDaemonThread.java
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>