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 ?

Thu Jul 5 18:27:15 UTC 2018

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?

..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));
>>>>>> <------
>>>
>>> ---
>>>>>
>>>>> ---------------------------
>
>