RFR JDK-8206394 : ResourceMark in AOTCompiledMethod::metadata_do, AOTCompiledMethod::clear_inline_caches , CompiledMethod::clear_ic_stubs , CompiledMethod::cleanup_inline_caches_impl - was : RE: ResourceMarks when CompiledIC_at is used ?
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Jul 9 15:30:48 UTC 2018
On 7/5/18 2:27 PM, Thomas Stüfe wrote:
> Hi Coleen,
>
> On Thu, Jul 5, 2018 at 7:29 PM, <coleen.phillimore at oracle.com> wrote:
>> Some ResourceMarks are missing because they might clear other resource
>> allocated memory when they return. This usually occurs with the
>> print_on(outputStream*) methods because logStream or stringStream may be
>> expanded in the print function and you don't want it cleared on return.
> Not LogStream, we explicitly refactored that to not use ResourceArea
> anymore: https://bugs.openjdk.java.net/browse/JDK-8181917
>
> I think stringStream should not use ResourceArea either. One should
> avoid handing pointers to resourceArea up and down the stack so much.
> For stringStream this is especially evil since whether this
> crashes/asserts depends on when the buffer is resized, and that
> depends on the content written to the stream, and that is highly
> runtime dependent and difficult to test.
>
> If we keep doing this, maybe we could at least modify
> stringStream::write() to assert on the ResourceMark equality every
> time it is called instead of just when the buffer gets resized. What
> do you think?
I have to confess that I wasn't paying attention to the LogStream
change, but it seems that doing the same sort of thing for stringStream
would be a good idea.
Thanks,
Coleen
>
> ..Thomas
>
>> There may also be a ResourceMark further up the call chain for these cases.
>> So you have to be careful with this.
>>
>>
>> On 7/5/18 12:18 PM, Baesken, Matthias wrote:
>>> Great , thanks for your help !
>>>
>>>
>>> Btw. while looking a bit more at the ResourceMark usages , I found
>>> that the name_and_sig_as_C_string() calls are mostly
>>> in the codebase with a related ResourceMark .
>>>
>>> However it is not done always, for example ; do you think it is missing
>>> at these places or not needed?
>>>
>>>
>>> hotspot/share/classfile/verifier.cpp#626
>>>
>>> 623void ClassVerifier::verify_method(const methodHandle& m, TRAPS) {
>>> 624 HandleMark hm(THREAD);
>>
>> The HandleMark is probably not needed here though. A ResourceMark does seem
>> like a good idea here.
>>
>> Coleen
>>
>>> 625 _method = m; // initialize _method
>>> 626 log_info(verification)("Verifying method %s",
>>> m->name_and_sig_as_C_string());
>>>
>>>
>>> hotspot/share/classfile/classLoader.cpp#2045
>>>
>>> void ClassLoader::compile_the_world_in(char* name, Handle loader, TRAPS) {
>>>
>>> 2058 if (HAS_PENDING_EXCEPTION) {
>>> 2059 clear_pending_exception_if_not_oom(CHECK);
>>> 2060 tty->print_cr("CompileTheWorld (%d) : Skipping
>>> method: %s", _compile_the_world_class_counter,
>>> m->name_and_sig_as_C_string());
>>> 2061 } else {
>>> 2062 _compile_the_world_method_counter++;
>>> 2063 }
>>> 2064 }
>>> 2065 } else {
>>> 2066 tty->print_cr("CompileTheWorld (%d) : Skipping method:
>>> %s", _compile_the_world_class_counter, m->name_and_sig_as_C_string());
>>> 2067 }
>>>
>>> Best regards, Matthias
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Donnerstag, 5. Juli 2018 18:07
>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>>>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>>>> Subject: Re: RFR JDK-8206394 : ResourceMark in
>>>> AOTCompiledMethod::metadata_do,
>>>> AOTCompiledMethod::clear_inline_caches ,
>>>> CompiledMethod::clear_ic_stubs ,
>>>> CompiledMethod::cleanup_inline_caches_impl - was : RE: ResourceMarks
>>>> when CompiledIC_at is used ?
>>>>
>>>> Looks good. I will test it and push if it passed.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/5/18 2:14 AM, Baesken, Matthias wrote:
>>>>> Hi Vladimir, thanks for looking into it !
>>>>>
>>>>> Please review :
>>>>>
>>>>> http://cr.openjdk.java.net/~mbaesken/webrevs/8206394/
>>>>>
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8206394
>>>>>
>>>>>
>>>>> Best regards, Matthias
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>>>> Sent: Mittwoch, 4. Juli 2018 19:43
>>>>>> To: Baesken, Matthias <matthias.baesken at sap.com>; 'hotspot-
>>>>>> dev at openjdk.java.net' <hotspot-dev at openjdk.java.net>
>>>>>> Subject: Re: ResourceMarks when CompiledIC_at is used ?
>>>>>>
>>>>>> Thank you, Matthias
>>>>>>
>>>>>> Yes, I checked all these places and they have to be fixed. Please, file
>>>>>> a bug for jdk11.
>>>>>>
>>>>>> Vladimir
>>>>>>
>>>>>> On 7/4/18 7:44 AM, Baesken, Matthias wrote:
>>>>>>> Hello, I recently looked at 8164293: HotSpot leaking memory in
>>>>>>> long-
>>>>>> running requests
>>>>>>> http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/rev/8dfbb002197a
>>>>>>>
>>>>>>> where a number of missing ResourceMarks were added .
>>>>>>> I think some of those ResourceMarks were added because of
>>>>>> CompiledIC_at in the changed functions .
>>>>>>> I checked for other CompiledIC_at in the hs coding , and found
>>>>>>> some
>>>>>> other places (see below) where ResourceMarks are missing as well.
>>>>>>> Should they be added ?
>>>>>>>
>>>>>>> Best regards, Matthias
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> src/hotspot/share/aot/aotCompiledMethod.cpp
>>>>>>>
>>>>>>> void AOTCompiledMethod::metadata_do(void f(Metadata*)) {
>>>>>>> .....
>>>>>>> 274 } else if (iter.type() == relocInfo::virtual_call_type) {
>>>>>>> 275 // Check compiledIC holders associated with this nmethod
>>>>>>> 276 CompiledIC *ic = CompiledIC_at(&iter);
>>>>>>> <---------------------------
>>>> ----
>>>>>> -----
>>>>>>>
>>>>>>> and
>>>>>>>
>>>>>>> 441void AOTCompiledMethod::clear_inline_caches() {
>>>>>>> 442 assert(SafepointSynchronize::is_at_safepoint(), "cleaning of IC's
>>>> only
>>>>>> allowed at safepoint");
>>>>>>> 443 if (is_zombie()) {
>>>>>>> 444 return;
>>>>>>> 445 }
>>>>>>> 446
>>>>>>> 447 RelocIterator iter(this);
>>>>>>> 448 while (iter.next()) {
>>>>>>> 449 iter.reloc()->clear_inline_cache();
>>>>>>> 450 if (iter.type() == relocInfo::opt_virtual_call_type) {
>>>>>>> 451 CompiledIC* cic = CompiledIC_at(&iter);
>>>>>>> <---------------------------
>>>> ----
>>>>>> -----
>>>>>>> 452 assert(cic->is_clean(), "!");
>>>>>>> 453 nativePltCall_at(iter.addr())->set_stub_to_clean();
>>>>>>> 454 }
>>>>>>> 455 }
>>>>>>> 456}
>>>>>>>
>>>>>>> src/hotspot/share/code/compiledMethod.cpp
>>>>>>>
>>>>>>> 326void CompiledMethod::clear_ic_stubs() {
>>>>>>> 327 assert_locked_or_safepoint(CompiledIC_lock);
>>>>>>> 328 RelocIterator iter(this);
>>>>>>> 329 while(iter.next()) {
>>>>>>> 330 if (iter.type() == relocInfo::virtual_call_type) {
>>>>>>> 331 CompiledIC* ic = CompiledIC_at(&iter);
>>>>>>> <---------------------------
>>>> ----
>>>>>> -----
>>>>>>> 332 ic->clear_ic_stub();
>>>>>>> 333 }
>>>>>>> 334 }
>>>>>>> 335}
>>>>>>>
>>>>>>>
>>>>>>> 547bool CompiledMethod::cleanup_inline_caches_impl(bool parallel,
>>>> bool
>>>>>> unloading_occurred, bool clean_all) {
>>>>>>> 548 assert_locked_or_safepoint(CompiledIC_lock);
>>>>>>> 549 bool postponed = false;
>>>>>>> 550
>>>>>>> 551 // Find all calls in an nmethod and clear the ones that point to
>>>>>>> non-
>>>>>> entrant,
>>>>>>> 552 // zombie and unloaded nmethods.
>>>>>>> 553 RelocIterator iter(this, oops_reloc_begin());
>>>>>>> 554 while(iter.next()) {
>>>>>>> 555
>>>>>>> 556 switch (iter.type()) {
>>>>>>> 557
>>>>>>> 558 case relocInfo::virtual_call_type:
>>>>>>> 559 if (unloading_occurred) {
>>>>>>> 560 // If class unloading occurred we first clear ICs where the
>>>>>>> cached
>>>>>> metadata
>>>>>>> 561 // is referring to an unloaded klass or method.
>>>>>>> 562 clean_ic_if_metadata_is_dead(CompiledIC_at(&iter));
>>>>>>> <------
>>>> ---
>>>>>> ---------------------------
>>
More information about the hotspot-dev
mailing list