8251261: CDS dumping should not clear states in live classes #227
Conversation
|
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
|
@iklam The following label will be automatically applied to this pull request: When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing list. If you would like to change these labels, use the |
|
/label add hotspot-runtime |
|
@iklam |
|
@iklam |
Webrevs
|
|
@iklam This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 14 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
| @@ -398,8 +398,6 @@ void KlassSubGraphInfo::add_subgraph_object_klass(Klass* orig_k, Klass *relocate | |||
| new(ResourceObj::C_HEAP, mtClass) GrowableArray<Klass*>(50, mtClass); | |||
| } | |||
|
|
|||
| assert(relocated_k->is_shared(), "must be a shared class"); | |||
There was a problem hiding this comment.
Why is this assert removed?
There was a problem hiding this comment.
The delete is reasonable since line 293 already assert that relocated_k is a relocated klass in shared region which must be shared class I think.
There was a problem hiding this comment.
Klass::set_is_shared() is called in Klass::remove_unshareable_info(). This PR has moved the calls to remove_unshareable_info() to a later stage. So when this assert is executed, relocated_k->is_shared() is false.
In the new commit 03f7485 I've restored the assert and changed it to the following:
assert(ArchiveBuilder::singleton()->is_in_buffer_space(relocated_k), "must be a shared class");
|
/integrate |
openjdk
bot
added
integrated
|
@iklam Since your change was applied there have been 14 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 8b85c3a. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
After JDK-8283091, the loop below can be vectorized partially.
Statement 1 can be vectorized but statement 2 can't.
```
// int[] iArr; long[] lArrFld; int i1,i2;
for (i1 = 6; i1 < 227; i1++) {
iArr[i1] += lArrFld[i1]++; // statement 1
iArr[i1 + 1] -= (i2++); // statement 2
}
```
But we got incorrect results because the vector packs of iArr are
scheduled incorrectly like:
```
...
load_vector XMM1,[R8 + openjdk#16 + R11 << openjdk#2]
movl RDI, [R8 + openjdk#20 + R11 << openjdk#2] # int
load_vector XMM2,[R9 + openjdk#8 + R11 << openjdk#3]
subl RDI, R11 # int
vpaddq XMM3,XMM2,XMM0 ! add packedL
store_vector [R9 + openjdk#8 + R11 << openjdk#3],XMM3
vector_cast_l2x XMM2,XMM2 !
vpaddd XMM1,XMM2,XMM1 ! add packedI
addl RDI, openjdk#228 # int
movl [R8 + openjdk#20 + R11 << openjdk#2], RDI # int
movl RBX, [R8 + openjdk#24 + R11 << openjdk#2] # int
subl RBX, R11 # int
addl RBX, openjdk#227 # int
movl [R8 + openjdk#24 + R11 << openjdk#2], RBX # int
...
movl RBX, [R8 + openjdk#40 + R11 << openjdk#2] # int
subl RBX, R11 # int
addl RBX, openjdk#223 # int
movl [R8 + openjdk#40 + R11 << openjdk#2], RBX # int
movl RDI, [R8 + openjdk#44 + R11 << openjdk#2] # int
subl RDI, R11 # int
addl RDI, openjdk#222 # int
movl [R8 + openjdk#44 + R11 << openjdk#2], RDI # int
store_vector [R8 + openjdk#16 + R11 << openjdk#2],XMM1
...
```
simplified as:
```
load_vector iArr in statement 1
unvectorized loads/stores in statement 2
store_vector iArr in statement 1
```
We cannot pick the memory state from the first load for LoadI pack
here, as the LoadI vector operation must load the new values in memory
after iArr writes 'iArr[i1 + 1] - (i2++)' to 'iArr[i1 + 1]'(statement 2).
We must take the memory state of the last load where we have assigned
new values ('iArr[i1 + 1] - (i2++)') to the iArr array.
In JDK-8240281, we picked the memory state of the first load. Different
from the scenario in JDK-8240281, the store, which is dependent on an
earlier load here, is in a pack to be scheduled and the LoadI pack
depends on the last_mem. As designed[2], to schedule the StoreI pack,
all memory operations in another single pack should be moved in the same
direction. We know that the store in the pack depends on one of loads in
the LoadI pack, so the LoadI pack should be scheduled before the StoreI
pack. And the LoadI pack depends on the last_mem, so the last_mem must
be scheduled before the LoadI pack and also before the store pack.
Therefore, we need to take the memory state of the last load for the
LoadI pack here.
To fix it, the pack adds additional checks while picking the memory state
of the first load. When the store locates in a pack and the load pack
relies on the last_mem, we shouldn't choose the memory state of the
first load but choose the memory state of the last load.
[1]https://github.com/openjdk/jdk/blob/0ae834105740f7cf73fe96be22e0f564ad29b18d/src/hotspot/share/opto/superword.cpp#L2380
[2]https://github.com/openjdk/jdk/blob/0ae834105740f7cf73fe96be22e0f564ad29b18d/src/hotspot/share/opto/superword.cpp#L2232
Jira: ENTLLT-5482
Change-Id: I341d10b91957b60a1b4aff8116723e54083a5fb8
CustomizedGitHooks: yes
We had an issue when CDS dumped a static archive (java -Xshare:dump), it would call
Klass::remove_unshareable_info()too early. In one of the test failures, ZGC was still scanning the heap and stepped on a class whose mirror has been removed.The fix is to avoid modifying the states of the Java classes during -Xshare:dump. Instead, we call
Klass::remove_unshareable_info()only on the copy of the classes which are written into the archive. It's safe to do so because these copies are visible only to the CDS dumping code. They aren't accessible by the GC or any other subsystems.It turns out that we were already doing this for the dynamic archive. So I just generalized the code in dynamicArchive.cpp and moved it to archiveBuilder.cpp. So this PR is one step forward for JDK-8234693 Consolidate CDS static and dynamic archive dumping code.
I also fixed another case where we modify the global VM state -- I removed
Universe::clear_basic_type_mirrors().We are still modifying some global VM states (such as SystemDictionary::_well_known_klasses). They seem harmless now, but we might have to do more fixes in the future.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/227/head:pull/227$ git checkout pull/227