From 59ae9b3407c359601cf6995070fdfa3cc161a1ac Mon Sep 17 00:00:00 2001 From: ysr Date: Mon, 1 Feb 2010 17:29:01 -0800 Subject: [PATCH] 6904516: More object array barrier fixes, following up on 6906727 Summary: Fixed missing pre-barrier calls for G1, modified C1 to call pre- and correct post-barrier interfaces, deleted obsolete interface, (temporarily) disabled redundant deferred barrier in BacktraceBuilder. Reviewed-by: coleenp, jmasa, kvn, never --- src/share/vm/c1/c1_Runtime1.cpp | 24 ++++++++++++------- src/share/vm/classfile/javaClasses.cpp | 17 ++++++++++--- .../gc_implementation/g1/g1CollectedHeap.cpp | 2 ++ src/share/vm/memory/barrierSet.hpp | 2 -- src/share/vm/memory/barrierSet.inline.hpp | 18 ++++---------- src/share/vm/runtime/stubRoutines.cpp | 12 +++++++++- 6 files changed, 47 insertions(+), 28 deletions(-) diff --git a/src/share/vm/c1/c1_Runtime1.cpp b/src/share/vm/c1/c1_Runtime1.cpp index 9093885ce..98738ed4b 100644 --- a/src/share/vm/c1/c1_Runtime1.cpp +++ b/src/share/vm/c1/c1_Runtime1.cpp @@ -1075,6 +1075,7 @@ enum { }; +// Below length is the # elements copied. template int obj_arraycopy_work(oopDesc* src, T* src_addr, oopDesc* dst, T* dst_addr, int length) { @@ -1083,22 +1084,22 @@ template int obj_arraycopy_work(oopDesc* src, T* src_addr, // barrier. The assert will fail if this is not the case. // Note that we use the non-virtual inlineable variant of write_ref_array. BarrierSet* bs = Universe::heap()->barrier_set(); - assert(bs->has_write_ref_array_opt(), - "Barrier set must have ref array opt"); + assert(bs->has_write_ref_array_opt(), "Barrier set must have ref array opt"); + assert(bs->has_write_ref_array_pre_opt(), "For pre-barrier as well."); if (src == dst) { // same object, no check + bs->write_ref_array_pre(dst_addr, length); Copy::conjoint_oops_atomic(src_addr, dst_addr, length); - bs->write_ref_array(MemRegion((HeapWord*)dst_addr, - (HeapWord*)(dst_addr + length))); + bs->write_ref_array((HeapWord*)dst_addr, length); return ac_ok; } else { klassOop bound = objArrayKlass::cast(dst->klass())->element_klass(); klassOop stype = objArrayKlass::cast(src->klass())->element_klass(); if (stype == bound || Klass::cast(stype)->is_subtype_of(bound)) { // Elements are guaranteed to be subtypes, so no check necessary + bs->write_ref_array_pre(dst_addr, length); Copy::conjoint_oops_atomic(src_addr, dst_addr, length); - bs->write_ref_array(MemRegion((HeapWord*)dst_addr, - (HeapWord*)(dst_addr + length))); + bs->write_ref_array((HeapWord*)dst_addr, length); return ac_ok; } } @@ -1162,9 +1163,16 @@ JRT_LEAF(void, Runtime1::oop_arraycopy(HeapWord* src, HeapWord* dst, int num)) #endif if (num == 0) return; - Copy::conjoint_oops_atomic((oop*) src, (oop*) dst, num); BarrierSet* bs = Universe::heap()->barrier_set(); - bs->write_ref_array(MemRegion(dst, dst + num)); + assert(bs->has_write_ref_array_opt(), "Barrier set must have ref array opt"); + assert(bs->has_write_ref_array_pre_opt(), "For pre-barrier as well."); + if (UseCompressedOops) { + bs->write_ref_array_pre((narrowOop*)dst, num); + } else { + bs->write_ref_array_pre((oop*)dst, num); + } + Copy::conjoint_oops_atomic((oop*) src, (oop*) dst, num); + bs->write_ref_array(dst, num); JRT_END diff --git a/src/share/vm/classfile/javaClasses.cpp b/src/share/vm/classfile/javaClasses.cpp index 173f2e26e..730f1c7ab 100644 --- a/src/share/vm/classfile/javaClasses.cpp +++ b/src/share/vm/classfile/javaClasses.cpp @@ -1121,10 +1121,23 @@ class BacktraceBuilder: public StackObj { } void flush() { + // The following appears to have been an optimization to save from + // doing a barrier for each individual store into the _methods array, + // but rather to do it for the entire array after the series of writes. + // That optimization seems to have been lost when compressed oops was + // implemented. However, the extra card-marks below was left in place, + // but is now redundant because the individual stores into the + // _methods array already execute the barrier code. CR 6918185 has + // been filed so the original code may be restored by deferring the + // barriers until after the entire sequence of stores, thus re-enabling + // the intent of the original optimization. In the meantime the redundant + // card mark below is now disabled. if (_dirty && _methods != NULL) { +#if 0 BarrierSet* bs = Universe::heap()->barrier_set(); assert(bs->has_write_ref_array_opt(), "Barrier set must have ref array opt"); bs->write_ref_array((HeapWord*)_methods->base(), _methods->length()); +#endif _dirty = false; } } @@ -1168,9 +1181,7 @@ class BacktraceBuilder: public StackObj { method = mhandle(); } - _methods->obj_at_put(_index, method); - // bad for UseCompressedOops - // *_methods->obj_at_addr(_index) = method; + _methods->obj_at_put(_index, method); _bcis->ushort_at_put(_index, bci); _index++; _dirty = true; diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp index c3319d13e..0c73f4d6c 100644 --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp +++ b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp @@ -2505,6 +2505,7 @@ G1CollectedHeap* G1CollectedHeap::heap() { } void G1CollectedHeap::gc_prologue(bool full /* Ignored */) { + // always_do_update_barrier = false; assert(InlineCacheBuffer::is_empty(), "should have cleaned up ICBuffer"); // Call allocation profiler AllocationProfiler::iterate_since_last_gc(); @@ -2518,6 +2519,7 @@ void G1CollectedHeap::gc_epilogue(bool full /* Ignored */) { // is set. COMPILER2_PRESENT(assert(DerivedPointerTable::is_empty(), "derived pointer present")); + // always_do_update_barrier = true; } void G1CollectedHeap::do_collection_pause() { diff --git a/src/share/vm/memory/barrierSet.hpp b/src/share/vm/memory/barrierSet.hpp index c624f2506..4188e11fa 100644 --- a/src/share/vm/memory/barrierSet.hpp +++ b/src/share/vm/memory/barrierSet.hpp @@ -124,8 +124,6 @@ public: // Below length is the # array elements being written virtual void write_ref_array_pre( oop* dst, int length) {} virtual void write_ref_array_pre(narrowOop* dst, int length) {} - // Below MemRegion mr is expected to be HeapWord-aligned - inline void write_ref_array(MemRegion mr); // Below count is the # array elements being written, starting // at the address "start", which may not necessarily be HeapWord-aligned inline void write_ref_array(HeapWord* start, size_t count); diff --git a/src/share/vm/memory/barrierSet.inline.hpp b/src/share/vm/memory/barrierSet.inline.hpp index ddc398348..f8858253e 100644 --- a/src/share/vm/memory/barrierSet.inline.hpp +++ b/src/share/vm/memory/barrierSet.inline.hpp @@ -42,16 +42,6 @@ void BarrierSet::write_ref_field(void* field, oop new_val) { } } -void BarrierSet::write_ref_array(MemRegion mr) { - assert((HeapWord*)align_size_down((uintptr_t)mr.start(), HeapWordSize) == mr.start() , "Unaligned start"); - assert((HeapWord*)align_size_up ((uintptr_t)mr.end(), HeapWordSize) == mr.end(), "Unaligned end" ); - if (kind() == CardTableModRef) { - ((CardTableModRefBS*)this)->inline_write_ref_array(mr); - } else { - write_ref_array_work(mr); - } -} - // count is number of array elements being written void BarrierSet::write_ref_array(HeapWord* start, size_t count) { assert(count <= (size_t)max_intx, "count too large"); @@ -61,12 +51,12 @@ void BarrierSet::write_ref_array(HeapWord* start, size_t count) { // strictly necessary for current uses, but a case of good hygiene and, // if you will, aesthetics) and the second upward (this is essential for // current uses) to a HeapWord boundary, so we mark all cards overlapping - // this write. In the event that this evolves in the future to calling a + // this write. If this evolves in the future to calling a // logging barrier of narrow oop granularity, like the pre-barrier for G1 // (mentioned here merely by way of example), we will need to change this - // interface, much like the pre-barrier one above, so it is "exactly precise" - // (if i may be allowed the adverbial redundancy for emphasis) and does not - // include narrow oop slots not included in the original write interval. + // interface, so it is "exactly precise" (if i may be allowed the adverbial + // redundancy for emphasis) and does not include narrow oop slots not + // included in the original write interval. HeapWord* aligned_start = (HeapWord*)align_size_down((uintptr_t)start, HeapWordSize); HeapWord* aligned_end = (HeapWord*)align_size_up ((uintptr_t)end, HeapWordSize); // If compressed oops were not being used, these should already be aligned diff --git a/src/share/vm/runtime/stubRoutines.cpp b/src/share/vm/runtime/stubRoutines.cpp index 25c02cd5f..22b1df868 100644 --- a/src/share/vm/runtime/stubRoutines.cpp +++ b/src/share/vm/runtime/stubRoutines.cpp @@ -196,11 +196,19 @@ void stubRoutines_init2() { StubRoutines::initialize2(); } // Default versions of arraycopy functions // +static void gen_arraycopy_barrier_pre(oop* dest, size_t count) { + assert(count != 0, "count should be non-zero"); + assert(count <= (size_t)max_intx, "count too large"); + BarrierSet* bs = Universe::heap()->barrier_set(); + assert(bs->has_write_ref_array_pre_opt(), "Must have pre-barrier opt"); + bs->write_ref_array_pre(dest, (int)count); +} + static void gen_arraycopy_barrier(oop* dest, size_t count) { assert(count != 0, "count should be non-zero"); BarrierSet* bs = Universe::heap()->barrier_set(); assert(bs->has_write_ref_array_opt(), "Barrier set must have ref array opt"); - bs->write_ref_array(MemRegion((HeapWord*)dest, (HeapWord*)(dest + count))); + bs->write_ref_array((HeapWord*)dest, count); } JRT_LEAF(void, StubRoutines::jbyte_copy(jbyte* src, jbyte* dest, size_t count)) @@ -240,6 +248,7 @@ JRT_LEAF(void, StubRoutines::oop_copy(oop* src, oop* dest, size_t count)) SharedRuntime::_oop_array_copy_ctr++; // Slow-path oop array copy #endif // !PRODUCT assert(count != 0, "count should be non-zero"); + gen_arraycopy_barrier_pre(dest, count); Copy::conjoint_oops_atomic(src, dest, count); gen_arraycopy_barrier(dest, count); JRT_END @@ -281,6 +290,7 @@ JRT_LEAF(void, StubRoutines::arrayof_oop_copy(HeapWord* src, HeapWord* dest, siz SharedRuntime::_oop_array_copy_ctr++; // Slow-path oop array copy #endif // !PRODUCT assert(count != 0, "count should be non-zero"); + gen_arraycopy_barrier_pre((oop *) dest, count); Copy::arrayof_conjoint_oops(src, dest, count); gen_arraycopy_barrier((oop *) dest, count); JRT_END -- GitLab