-
Notifications
You must be signed in to change notification settings - Fork 38
Concurrent Immix #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Concurrent Immix #311
Conversation
openjdk/barriers/mmtkSATBBarrier.cpp
Outdated
@@ -0,0 +1,536 @@ | |||
#define private public // too lazy to change openjdk... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied from lxr branch. Basically when code needs to be patched, we need to call a private method. A proper solution is to define friend class in OpenJDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works around the fact that LIRAssembler::as_Address
is private in OpenJDK 11. It is a public method in OpenJDK 21. We may remove this workaround when porting.
#define __ ideal. | ||
|
||
void MMTkSATBBarrierSetC2::object_reference_write_pre(GraphKit* kit, Node* src, Node* slot, Node* pre_val, Node* val) const { | ||
if (can_remove_barrier(kit, &kit->gvn(), src, slot, val, /* skip_const_null */ false)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful of barrier elimination when implementing this in the binding. target == null does not mean the barrier can be eliminated. This bug takes me a while to find out.
openjdk/barriers/mmtkSATBBarrier.hpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this mmtkSATBBarrier is also an object remembering barrier. Theoretically, we should be able to reuse the existing mmtkObjectBarrier. But due to the current barrier api design, the mmtkObjectBarrier has the generational semantic baked in.
_pre_barrier_c1_runtime_code_blob = Runtime1::generate_blob(buffer_blob, -1, "mmtk_pre_write_code_gen_cl", false, &pre_write_code_gen_cl); | ||
MMTkPostBarrierCodeGenClosure post_write_code_gen_cl; | ||
_post_barrier_c1_runtime_code_blob = Runtime1::generate_blob(buffer_blob, -1, "mmtk_post_write_code_gen_cl", false, &post_write_code_gen_cl); | ||
// MMTkBarrierCodeGenClosure write_code_gen_cl_patch_fix(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was in the lxr branch and after I discussed with wenyu, we believe it is redundant. The code patching has already been dealt with, no need to do any special things here
openjdk/mmtkBarrierSet.hpp
Outdated
@@ -70,13 +72,16 @@ class MMTkBarrierSetRuntime: public CHeapObj<mtGC> { | |||
static void object_reference_array_copy_pre_call(void* src, void* dst, size_t count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In OpenJDK, array copy does not give base address of src or dst arrays, so nothing can be remembered. I have a patch in Iso to pass down base address of both src and dst arrarys (they are required by the publication semantics). I am not sure if it is worthwhile to have that in our OpenJDK fork
All the tests failed. @tianleq Is there anything we need to do for running ConcurrentImmix? |
I did not test compressed pointers as in Iso, it is not supported yet. Other than that, the minheap is much larger due to it being non-moving immix |
Thanks. I temporarily disabled compressed pointer for concurrent Immix. We should get compressed pointer working before merging the PR. |
openjdk/barriers/mmtkSATBBarrier.cpp
Outdated
constexpr int kUnloggedValue = 1; | ||
|
||
static inline intptr_t side_metadata_base_address() { | ||
return UseCompressedOops ? SATB_METADATA_BASE_ADDRESS : SATB_METADATA_BASE_ADDRESS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is problematic, and I believe the crash is due to this. If compressed pointers is enabled, then probably we need a different base address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address of unlog bits is unrelated to whether we use compressed oops or not. It should be just SATB_METADATA_BASE_ADDRESS
OOM is expected, as concurrent Immix is non-moving. Tianle and Wenyu clarified the issue about compressed pointer. Currently the SATB barrier computes metadata address differently, based on whether compressed pointer is in used or not, which is incorrect. We should use the same approach as how metadata is computed in the object barrier. The current implementation of SATB barrier is derived from lxr which uses field barrier, and needs to deal with the difference of field slots with/without compressed pointer. |
ea4c968
to
fc9d143
Compare
fc9d143
to
5018b70
Compare
Current CI runs concurrent Immix with 4x min heap. There are still some benchmarks that ran out of memory. I will increase it to 5x. There are a few correctness issues. Segfault in mmtk_openjdk::abi::OopDesc::size: fastdebug h2, fastdebug jython
old_queue is not empty: fastdebug pmd
Segfault in mark_lines_for_object: release h2, release jythonThis could be the same issue as the first one (
|
I am aware of 4x is not enough, jython OOM even with 5x, this is true in the stop-the-world non-moving immix. The iython crash is what I saw when barrier is incorrectly eliminated. I fixed that and never saw it again. I also look at my test run, only see OOMs on jython. I will try to reproduce those locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the code were copied from the field logging barrier in the lxr
branch. When not using compressed oops, each field is 64 bits, and the granularity of field-logging bits is one bit per 8 bytes. But when using compressed oops, fields become 32 bits, and the field-logging bits becomes one bit per 4 bytes. That's why the shift changes between 5 or 6 depending on whether compressed oops is enabled.
But here we are working on the object-remembering barrier, and using the regular global unlog bits. Its granularity is only related to the object alignment, not the field size. On 64-bit machines, objects are always 64-bit aligned. So we should always shift by 6 when computing the address of unlog bits. Similarly, when computing the in-byte shift, we should always shift by 3.
Change this and the segmentation fault disappears, even when using compressed oops.
openjdk/barriers/mmtkSATBBarrier.cpp
Outdated
constexpr int kUnloggedValue = 1; | ||
|
||
static inline intptr_t side_metadata_base_address() { | ||
return UseCompressedOops ? SATB_METADATA_BASE_ADDRESS : SATB_METADATA_BASE_ADDRESS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The address of unlog bits is unrelated to whether we use compressed oops or not. It should be just SATB_METADATA_BASE_ADDRESS
|
I also observed a crash during a full GC (after patching the shifting operations for unlog bits as I suggested above).
It has something to do with finalizers. |
The initial implementation does not schedule finalization related packets in final pause. This commit should have fixed that: mmtk/mmtk-core@2bbb200 |
This reverts commit b120bae.
This commit follows what G1 does for pre write barrier: https://github.com/mmtk/openjdk/blob/28e56ee32525c32c5a88391d0b01f24e5cd16c0f/src/hotspot/share/gc/g1/g1BarrierSet.inline.hpp#L38. Just skip the barrier for |
This doesn't work due to #313. I just changed MMTk core to work around the issue in #311 (comment). |
We will introduce it later when we implement a plan that needs it.
... instead of from a mutator instance.
- "jdk11-master|ms|s|fail_on_oom|tph|preserve|mmtk_gc-StickyImmix" | ||
- "jdk11-master|ms|s|fail_on_oom|tph|preserve|mmtk_gc-MarkSweep" | ||
- "jdk11-master|ms|s|fail_on_oom|tph|preserve|mmtk_gc-MarkCompact" | ||
# TODO: We need to disable compressed oops for Compressor temporarily until it supports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is using the old version of the CI script where we didn't have compressed pointer support for Compressor. This needs to get updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you haven't pushed this change, but the current PR doesn't have this fixed.
IdealKit ideal(access.kit(), true); | ||
uint alias_idx = access.kit()->C->get_alias_index(access.addr().type()); | ||
Node* pre_val = ideal.load(ideal.ctrl(), access.addr().node(), static_cast<const TypeOopPtr*>(val.type()), access.type(), alias_idx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it eagerly load instead of letting this->object_reference_write_pre
decide whether the load is necessary? Suppose this
is MMTkObjectBarrierSetC2
. When storing into an oop field, access.is_oop()
will be true, and it will perform the load operation. Then MMTkObjectBarrierSetC2::object_reference_write_pre
is a no-op, and the loaded result pre_val
is of no use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre_val
is not even used inside MMTkSATBBarrierSetC2::object_reference_write_pre
, so I think this can be removed completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I used to have a G1 style SATB barrier, and it needs the pre_val
Node* result = BarrierSetC2::atomic_xchg_at_resolved(access, new_val, value_type); | ||
if (access.is_oop()) { | ||
object_reference_write_pre(access.kit(), access.base(), access.addr().node(), result, new_val); | ||
object_reference_write_post(access.kit(), access.base(), access.addr().node(), new_val); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong. This object-remembering SATB pre-barrier is not an "old-value barrier", but a "before-modification barrier". The object_reference_write_pre
will scan the object and record the current value of all fields. Because atomic xchg always succeeds, the object_reference_write_pre
is guaranteed to see new_val
instead of the old value, which is wrong. The SATB pre-barrier should see the old value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I copied from G1 and since now our SATB barrier is an object remembering barrier, this becomes irrelevant. I do not think it is wrong, it is just unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Draft PR for concurrent immix