RFR: 8331208: Memory stress test that checks OutOfMemoryError stack trace fails [v2]

Thu May 2 08:50:56 UTC 2024

On Wed, 1 May 2024 13:22:19 GMT, Doug Simon <dnsimon at openjdk.org> wrote:

>> This pull request mitigates failures in memory stress tests that check the stack trace of an `OutOfMemoryError` for certain expected entries.
>> 
>> The stack trace of an OOME will [not be allocated once all preallocated OOMEs are used up](https://github.com/openjdk/jdk/blob/3d5eeac3a38ece4a23ea6da2dfe5939d64e81cea/src/hotspot/share/memory/universe.cpp#L722). If the only heap allocations performed in stressful conditions are those of the stress test, then the [4 preallocated OOMEs](https://github.com/openjdk/jdk/blob/f1d0e715b67e2ca47b525069d8153abbb33f75b9/src/hotspot/share/runtime/globals.hpp#L800) would be sufficient. However, it's possible for VM internal allocations to also occur during stressful conditions, especially in `-Xcomp` mode. For example, [CompileBroker::compile_method](https://github.com/openjdk/jdk/blob/3d5eeac3a38ece4a23ea6da2dfe5939d64e81cea/src/hotspot/share/compiler/compileBroker.cpp#L1399) will try to resolve the string constants in the constant pool of the method about to be compiled. This can fail as shown here:
>> 
>> V  [jvm.dll+0x62c23a]  Exceptions::_throw+0x11a  (exceptions.cpp:168)
>> V  [jvm.dll+0x62d85b]  Exceptions::_throw_oop+0xab  (exceptions.cpp:140)
>> V  [jvm.dll+0xbbce78]  MemAllocator::Allocation::check_out_of_memory+0x208  (memAllocator.cpp:138)
>> V  [jvm.dll+0xbbcac8]  MemAllocator::allocate+0x158  (memAllocator.cpp:377)
>> V  [jvm.dll+0x79bd05]  InstanceKlass::allocate_instance+0x95  (instanceKlass.cpp:1509)
>> V  [jvm.dll+0x7ddeed]  java_lang_String::basic_create+0x9d  (javaClasses.cpp:273)
>> V  [jvm.dll+0x7e43c0]  java_lang_String::create_from_unicode+0x60  (javaClasses.cpp:291)
>> V  [jvm.dll+0xdb91a5]  StringTable::do_intern+0xb5  (stringTable.cpp:379)
>> V  [jvm.dll+0xdba9f2]  StringTable::intern+0x1b2  (stringTable.cpp:368)
>> V  [jvm.dll+0xdbaaa6]  StringTable::intern+0x86  (stringTable.cpp:328)
>> V  [jvm.dll+0x51c8b1]  ConstantPool::string_at_impl+0x1d1  (constantPool.cpp:1251)
>> V  [jvm.dll+0x51b95b]  ConstantPool::resolve_string_constants_impl+0xeb  (constantPool.cpp:800)
>> V  [jvm.dll+0x4f2f8d]  CompileBroker::compile_method+0x31d  (compileBroker.cpp:1395)
>> V  [jvm.dll+0x4f3474]  CompileBroker::compile_method+0xc4  (compileBroker.cpp:1348)
>> 
>> These internal allocations can occur before the allocations of the test and thus use up the pre-allocated OOMEs. As a result, the OOMEs triggered by the stress test may end up throwing the [default, shared OOME instance](https://github.com/openjdk/jdk/blob/3d5eeac3a38ec...
>
> Doug Simon has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - don't perform XX actions for OOME when in scope of an InternalOOMEMark
>  - rename SandboxedOOMEMark to InternalOOMEMark

I took a look at this because it touches GC code, and therefore have a few nits / style requests related to that. However, don't consider this a full review since I'm not familiar with the part of the code / issues this PR intends to solve.

src/hotspot/share/gc/shared/memAllocator.cpp line 140:

> 138:     THROW_OOP_(exception, true);
> 139:   } else {
> 140:     THROW_OOP_(Universe::out_of_memory_error_java_heap(/* omit_backtrace*/ true), true);

Given that the only explicitly passed in value for `omit_backtrace` is `true` I think it would be nicer to create a separate function for this case, instead of having a comment always explaining what true stands for. Maybe `out_of_memory_error_java_heap_omit_backtrace()`?

src/hotspot/share/gc/shared/memAllocator.hpp line 131:

> 129: 
> 130:  public:
> 131:   InternalOOMEMark(JavaThread* thread) {

Suggestion:

  explicit InternalOOMEMark(JavaThread* thread) {

src/hotspot/share/memory/universe.cpp line 658:

> 656:     oome = gen_out_of_memory_error(oome);
> 657:   }
> 658:   return oome;

It could be nice to get rid of the double negation here:
Suggestion:

oop Universe::out_of_memory_error_java_heap(bool omit_backtrace) {
  oop oome = out_of_memory_errors()->obj_at(_oom_java_heap);
  if (omit_backtrace) {
    return oome;
  }
  return gen_out_of_memory_error(oome);

src/hotspot/share/memory/universe.hpp line 274:

> 272:   // may or may not have a backtrace. If error has a backtrace then the stack trace is already
> 273:   // filled in.
> 274:   static oop out_of_memory_error_java_heap(bool omit_backtrace=false);

Suggestion:

  static oop out_of_memory_error_java_heap(bool omit_backtrace = false);

src/hotspot/share/oops/klass.cpp line 881:

> 879:       THROW_OOP(Universe::out_of_memory_error_array_size());
> 880:     } else {
> 881:       THROW_OOP(Universe::out_of_memory_error_java_heap(/* omit_backtrace*/ true));

Suggestion:

      THROW_OOP(Universe::out_of_memory_error_java_heap(/* omit_backtrace */ true));

src/hotspot/share/runtime/javaThread.hpp line 57:

> 55: class JNIHandleBlock;
> 56: class JVMCIRuntime;
> 57: class InternalOOMEMark;

It would be nice to get this sorted as the other forward declarations.

src/hotspot/share/runtime/javaThread.hpp line 718:

> 716:   bool in_internal_oome_mark() const            { return _in_internal_oome_mark; }
> 717:   void set_in_internal_oome_mark(bool b)        { _in_internal_oome_mark = b;    }
> 718: 

Should all these be prefixed with `is` like:

  bool is_in_VTMS_transition() const             { return _is_in_VTMS_transition; }
  bool is_in_tmp_VTMS_transition() const         { return _is_in_tmp_VTMS_transition; }
  bool is_in_any_VTMS_transition() const { return _is_in_VTMS_transition || _is_in_tmp_VTMS_transition; }

-------------

PR Review: https://git.openjdk.org/jdk/pull/18925#pullrequestreview-2035108670
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587257272
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587241226
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587252177
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587252545
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587252963
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587248746
PR Review Comment: https://git.openjdk.org/jdk/pull/18925#discussion_r1587268629