-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8369013: Shenandoah: passive mode should support enabling ShenandoahCardBarrier #27966
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?
Conversation
|
👋 Welcome back rgithubli! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@rgithubli The following labels 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 lists. If you would like to change these labels, use the /label pull request command. |
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 turned out to be simpler than expected. Nice work. I suggest some small changes.
| // it means we're under passive mode and we have to initialize old gen | ||
| // for the purpose of having card table. | ||
| if (ShenandoahCardBarrier && !(mode()->is_generational())) { | ||
| _generation_sizer.heap_size_changed(max_capacity()); |
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 think we could simplify here by not using the _generation_sizer. It should be fine to pass max_capacity() instead of max_capacity_old here.
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.
Yeah that'd be simpler. Updating. Thanks.
| } | ||
|
|
||
| ShenandoahOldGeneration* old_generation() const { | ||
| assert(mode()->is_generational(), "Old generation requires generational mode"); |
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.
Can we assert that ShenandoahCardBarrier is on instead of removing this assertion?
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.
Sure
| ConcurrentGCTimer* _gc_timer; | ||
| // For exporting to SA | ||
| int _log_min_obj_alignment_in_bytes; | ||
| ShenandoahGenerationSizer _generation_sizer; |
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 don't think we need this here.
| void initialize_heuristics() override; | ||
|
|
||
| static ShenandoahGenerationalHeap* heap() { | ||
| shenandoah_assert_generational(); |
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.
Can we put these assertions back? I don't see any code in the PR that would invalidate these assertions. I also don't see any changes that would instantiate the generational heap for non generational mode, so if there is code trying to use the generational heap, its behavior will be undefined.
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 would crash at here with the assertion: code. Provided some of the stacktrace below. It's mostly because we enabled card barrier flag and the code just fell through and assumes it's generational.
Would it be more appropriate to change this line to be ShenandoahHeap* heap = ShenandoahHeap::heap();?
Stack: [0x00007f1082bd2000,0x00007f1082cd2000], sp=0x00007f1082cce370, free space=1008k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V [libjvm.so+0x13e0e90] NativeStackPrinter::print_stack(outputStream*, char*, int, unsigned char*&, bool
, int)+0x70 (shenandoahGenerationalHeap.hpp:47)
V [libjvm.so+0x18fd2e9] VMError::report(outputStream*, bool)+0x1bff (vmError.cpp:979)
V [libjvm.so+0x1900d01] VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*,
unsigned char*, void const*, void const*, char const*, int, unsigned long)+0x8b1 (vmError.cpp:1887)
V [libjvm.so+0xacf5ca] report_vm_status_error(char const*, int, char const*, int, char const*)+0x0 (deb
ug.cpp:196)
V [libjvm.so+0xacf33b] print_error_for_unit_test(char const*, char const*, __va_list_tag*)+0x0 (debug.c
pp:149)
V [libjvm.so+0x15bbec1] ShenandoahAsserts::assert_generational(char const*, int)+0x8b (shenandoahAssert
s.cpp:532)
V [libjvm.so+0x15be196] ShenandoahGenerationalHeap::heap()+0x19 (shenandoahGenerationalHeap.hpp:47)
V [libjvm.so+0x16993bc] void card_mark_barrier<narrowOop>(narrowOop*, oopDesc*)+0x73 (shenandoahReferenceProcessor.cpp:65)
V [libjvm.so+0x169a2ac] bool ShenandoahReferenceProcessor::discover<narrowOop>(oopDesc*, ReferenceType, unsigned int)+0x1da (shenandoahReferenceProcessor.cpp:409)
V [libjvm.so+0x1698a5d] ShenandoahReferenceProcessor::discover_reference(oopDesc*, ReferenceType)+0xed (shenandoahReferenceProcessor.cpp:433)
V [libjvm.so+0x1684aeb] bool InstanceRefKlass::try_discover<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ReferenceType, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x77 (instanceRefKlass.inline.hpp:74)
V [libjvm.so+0x1682bfc] void InstanceRefKlass::oop_oop_iterate_discovery<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>, AlwaysContains>(oopDesc*, ReferenceType, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, AlwaysContains&)+0x2c (instanceRefKlass.inline.hpp:84)
V [libjvm.so+0x1680acc] void InstanceRefKlass::oop_oop_iterate_ref_processing<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>, AlwaysContains>(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*, AlwaysContains&)+0x80 (instanceRefKlass.inline.hpp:111)
V [libjvm.so+0x167ec2e] void InstanceRefKlass::oop_oop_iterate_ref_processing<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x2c (instanceRefKlass.inline.hpp:134)
V [libjvm.so+0x167ba88] void InstanceRefKlass::oop_oop_iterate<narrowOop, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0> >(oopDesc*, ShenandoahMarkRefsClosure<(ShenandoahGenerationType)0>*)+0x16c (instanceRefKlass.inline.hpp:154)
WIP. Add card barriers to passive mode to test out the price of card barriers.
How this change is implemented is to instantiate the old region in passive mode - old region owns the card table so this would minimize the code change with a bit price of native memory. It does sound weird to have old gen in passive mode, but since passive mode is a just diagnostic mode, we'll go with it for the cleanliness of the change.
Pending tests. Probably also good to have a perf test to test card barrier cost?
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27966/head:pull/27966$ git checkout pull/27966Update a local copy of the PR:
$ git checkout pull/27966$ git pull https://git.openjdk.org/jdk.git pull/27966/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27966View PR using the GUI difftool:
$ git pr show -t 27966Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27966.diff