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

Thomas Stüfe thomas.stuefe at gmail.com
Sun Jun 24 17:02:07 UTC 2018


Hi Lois,

thanks for the explanations!

If you just change the comment, I do not need another webrev.

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


More information about the hotspot-runtime-dev mailing list