8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer#2312
8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer#2312fparain wants to merge 2 commits intoopenjdk:lworldfrom
Conversation
|
👋 Welcome back fparain! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
Arraying
left a comment
Arraying
left a comment
There was a problem hiding this comment.
Thanks for this change. This is a valuable thing to have in our test, and I think the logic of using a bitmap to check for overlaps makes a lot of sense. I've left some code feedback and potential future ideas.
| } | ||
| int fieldOffset = offset - cl.firstFieldOffset; | ||
| for (FieldBlock fb : cl.nonStaticFields) { | ||
| if (fb.isRegular() && fb.name().compareTo("\".empty\"") != 0) { |
There was a problem hiding this comment.
Nit: use .equals on the String.
| } | ||
| for (FieldBlock block : layout.nonStaticFields) { | ||
| if (block.isRegular() && block.name().compareTo("\".empty\"") != 0) { | ||
| for (int i = baseOffset + block.offset(); i < baseOffset +block.offset() + block.size(); i++) { |
| Asserts.assertTrue(lo.getCurrentLine().startsWith("Non-oop acmp map <offset,size>"), lo.getCurrentLine()); | ||
| String[] acmp_nonoop_line = lo.getCurrentLine().split("\\s+"); | ||
| Asserts.assertTrue(acmp_nonoop_line.length >= 4, "Wrong format for non-oop acmp map line: " + lo.getCurrentLine()); | ||
| for (int i = 4; i < acmp_nonoop_line.length; i++) { |
There was a problem hiding this comment.
Maybe more a matter of personal style, but I think this may too sensitive to changes in HotSpot. I would suggest to do the following:
String searchingFor = "Non-oop acmp map <offset,size>";
// assert current line contains searchingFor
String toParse = lo.getCurrentLine().substring(searchingFor.length);
String[] acmpNoopLine = toParse.split("\\s+");
// assert there is at least one
for (String tuple : acmpNoopLine) // ...The rationale being if I update this message, change indentation, etc. I don't want to spend too much time changing the index numbers everywhere in this test.
There was a problem hiding this comment.
I don't like the format of the log nor the way it is parsed (I have to blame myself for that).
The whole system could be reworked to have a log format that is both readable and easily parsable, combined with a real parser in the FieldLayoutAnalyzer. But this is beyond the scope of this PR.
| } | ||
| // Non-oop acmp map <offset,size>: <12,1> <16,4> ... | ||
| Asserts.assertTrue(lo.getCurrentLine().startsWith("Non-oop acmp map <offset,size>"), lo.getCurrentLine()); | ||
| String[] acmp_nonoop_line = lo.getCurrentLine().split("\\s+"); |
| } | ||
| // Non-oop acmp map <offset,size>: <12,1> <16,4> ... | ||
| Asserts.assertTrue(lo.getCurrentLine().startsWith("Non-oop acmp map <offset,size>"), lo.getCurrentLine()); | ||
| String[] acmp_nonoop_line = lo.getCurrentLine().split("\\s+"); |
There was a problem hiding this comment.
May be worth doing a .trim() before splitting by whitespace, it's good practice so we don't end up with any surprises, especially since our output has trailing spaces at the moment.
There was a problem hiding this comment.
The expression used to split the line takes care of removing consecutive whitespaces and trailing whitespaces.
| Asserts.assertTrue(lo.getCurrentLine().startsWith("oop acmp map"), lo.getCurrentLine()); | ||
| String[] acmp_oop_line = lo.getCurrentLine().split("\\s+"); | ||
| Asserts.assertTrue(acmp_oop_line.length >= 3, "Wrong format for oop acmp map line: " + lo.getCurrentLine()); | ||
| for (int i = 3; i < acmp_oop_line.length; i++) { |
There was a problem hiding this comment.
Same comments as with the non-OOP map apply here.
| } | ||
| int fieldOffset = offset - cl.firstFieldOffset; | ||
| for (FieldBlock fb : cl.nonStaticFields) { | ||
| if (fb.isRegular() && fb.name().compareTo("\".empty\"") != 0) { |
There was a problem hiding this comment.
Nit: prefer .equals rather than comparison.
| if (layoutKind.hasNullMarker()) { | ||
| Asserts.assertTrue(cl.hasNullMarker()); | ||
| int nullMarkerOffset = fieldOffset + cl.nullMarkerOffset; | ||
| Asserts.assertFalse(bitmap[nullMarkerOffset], "Overlap in null marker at offset " + nullMarkerOffset + " of class " + cl.name); |
There was a problem hiding this comment.
If we are in a nullable layout, concrete, and have no fields, the null marker is repurposed as .empty IIRC. In such a case, what does it get printed as? My concern is that we will see the null marker as .empty above, mark it, and then fail this assertion.
There was a problem hiding this comment.
The synthetic .empty field is filtered out when constructing the bitmap from fields information (lines 775 and 802), so its slot is available for the null-marker.
| for (ClassLayout layout : layouts) { | ||
| if (!layout.isValue) continue; | ||
| System.out.println("Checking acmp map of class " + layout.name); | ||
| boolean[] acmpBitMap = new boolean[layout.instanceSize]; |
There was a problem hiding this comment.
If I understand the format properly, it could be a nice addition to verify that the leftover false regions in the bitmap correspond to the padding BlockType.
There was a problem hiding this comment.
false regions could be padding, or empty, or reserved, in fact all block types that are not real basic types or the null marker.
| for (FieldBlock block : layout.nonStaticFields) { | ||
| if (block.isRegular() || block.isFlat() || block.type() == BlockType.INHERITED) { | ||
| if (block.offset() < firstOffset || firstOffset == 0) { | ||
| firstOffset = block.offset(); |
2 participants
Add acmp_maps verification to the FieldLayoutAnalyzer.
The FieldLayoutAnalyzer recomputes the memory segments used by fields from the available layout descriptions, and compare the results to the acmp_maps computed by the JVM.
The verification is automatically performed for all tests calling the
check()method of the FieldLayoutAnalyzer (like the ValueRandomLayoutTest.java).Tested repeatedly with ValueRandomLayoutTest.java on Mach5.
Thanks,
Fred
Progress
Error
- [x] I confirm that I make this contribution in accordance with the [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai).Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2312/head:pull/2312$ git checkout pull/2312Update a local copy of the PR:
$ git checkout pull/2312$ git pull https://git.openjdk.org/valhalla.git pull/2312/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2312View PR using the GUI difftool:
$ git pr show -t 2312Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2312.diff
Using Webrev
Link to Webrev Comment