SapMachine #2250: Add adjustments to heap dumps for the buildpack#2251
SapMachine #2250: Add adjustments to heap dumps for the buildpack#2251schmelter-sap wants to merge 9 commits into
Conversation
|
Hello @schmelter-sap, this pull request fulfills all formal requirements. |
|
Couple of minor adjustments might be needed in globals.hpp |
|
|
||
| // SAPJVM 2026-05-06: Fill with zeros, if we don't dump the whole content of the array. | ||
| if (fill_with_zero > 0) { | ||
| writer->write_zero((u4) fill_with_zero * type_size); |
There was a problem hiding this comment.
Should this be size_t like it is used in the declaration of write_zero ?
There was a problem hiding this comment.
The length in bytes in this function always uses u4. So for consistency with other code I kept it that way.
There was a problem hiding this comment.
I agree with @MBaesken . This can overflow and the write_zero function takes size_t anyways. So why do this cast to u4 here?
| "-XX:+HeapDumpOverwrite", | ||
| "-XX:HeapDumpParallelism=1"); | ||
| verifyDump(dumpFile, false); | ||
| Asserts.assertTrue(firstSegment.exists(), "Segment file should not have been created (parallelism=1)."); |
There was a problem hiding this comment.
If firstSegments is NOT there (exists fails), assertTrue fails; so should the message be better something like 'should exist but does not' ?
There was a problem hiding this comment.
This tests if the segment file was created during the heap dump. It would overwrite the existing file and is deleted when the heap dump is merged. So if the segment file was created, the fristSegment file doesn't exist anymore.
|
Hello @schmelter-sap, this pull request fulfills all formal requirements. |
| "the VM decide.") \ | ||
| \ | ||
| /* SAPJVM 2026-05-06: Allow to skip content of large arrays in dumps.*/ \ | ||
| product(bool, LimitPrimArrayContentInHeapDump, false, MANAGEABLE, \ |
There was a problem hiding this comment.
Maybe consider the naming: Primitive instead of Prim. The name is long anyways so we can use a precise name.
| "Sets the parallelism of the heap dump creation. 0 means to let "\ | ||
| "the VM decide.") \ | ||
| \ | ||
| /* SAPJVM 2026-05-06: Allow to skip content of large arrays in dumps.*/ \ |
There was a problem hiding this comment.
Should the SAPJVM comments be SapMachine?
| int length = calculate_array_max_length(writer, array, header_size); | ||
| int type_size = type2aelembytes(type); | ||
| // SapMachine 2026-05-06 | ||
| int fill_with_zero = 0; |
There was a problem hiding this comment.
We could move that to the next hunk (after line 1399), so we do not need this lonely comment, right?
|
|
||
| // SAPJVM 2026-05-06: Fill with zeros, if we don't dump the whole content of the array. | ||
| if (fill_with_zero > 0) { | ||
| writer->write_zero((u4) fill_with_zero * type_size); |
There was a problem hiding this comment.
I agree with @MBaesken . This can overflow and the write_zero function takes size_t anyways. So why do this cast to u4 here?
| } | ||
|
|
||
| class ArrayAllocOOMApp extends ArrayAllocApp { | ||
| public static int arraySize = 54321; |
There was a problem hiding this comment.
This is not needed / not used isn't it? The value used by allocArrays() seems to be in ArrayAllocApp.
| return; | ||
| } | ||
|
|
||
| // SAPJVM 2026-05-06: If enabled, we don't dump the whole content of large arrays, but just the start. |
There was a problem hiding this comment.
Still some 'SAPJVM' instead of SapMachine (at various source locations).
|
Hello @schmelter-sap, this pull request fulfills all formal requirements. |
4 participants
This change adds features to the heap dump for the cf buildpack to be able to store heap dumps for customers in the object store. It includes the following features:
fixes #2250