Skip to content
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

8269897: Shenandoah: Resolve UNKNOWN access strength, where possible #4697

Closed
wants to merge 6 commits into from
Closed
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -54,11 +54,11 @@ class ShenandoahBarrierSet: public BarrierSet {
static bool need_keep_alive_barrier(DecoratorSet decorators, BasicType type);

static bool is_strong_access(DecoratorSet decorators) {
return (decorators & (ON_WEAK_OOP_REF | ON_PHANTOM_OOP_REF | ON_UNKNOWN_OOP_REF)) == 0;
return (decorators & (ON_WEAK_OOP_REF | ON_PHANTOM_OOP_REF)) == 0;
}

static bool is_weak_access(DecoratorSet decorators) {
return (decorators & (ON_WEAK_OOP_REF | ON_UNKNOWN_OOP_REF)) != 0;
return (decorators & ON_WEAK_OOP_REF) != 0;
}

static bool is_phantom_access(DecoratorSet decorators) {
@@ -90,8 +90,6 @@ class ShenandoahBarrierSet: public BarrierSet {
inline void satb_enqueue(oop value);
inline void iu_barrier(oop obj);

template <DecoratorSet decorators>
inline void keep_alive_if_weak(oop value);
inline void keep_alive_if_weak(DecoratorSet decorators, oop value);

inline void enqueue(oop obj);
@@ -101,8 +99,17 @@ class ShenandoahBarrierSet: public BarrierSet {
template <class T>
inline oop load_reference_barrier_mutator(oop obj, T* load_addr);

template <DecoratorSet decorators, class T>
inline oop load_reference_barrier(oop obj, T* load_addr);
template <class T>
inline oop load_reference_barrier(DecoratorSet decorators, oop obj, T* load_addr);

template <typename T>
inline oop oop_load(DecoratorSet decorators, T* addr);

template <typename T>
inline oop oop_cmpxchg(DecoratorSet decorators, T* addr, oop compare_value, oop new_value);

template <typename T>
inline oop oop_xchg(DecoratorSet decorators, T* addr, oop new_value);

private:
template <class T>
@@ -125,7 +132,6 @@ class ShenandoahBarrierSet: public BarrierSet {
template <DecoratorSet decorators, typename BarrierSetT = ShenandoahBarrierSet>
class AccessBarrier: public BarrierSet::AccessBarrier<decorators, BarrierSetT> {
typedef BarrierSet::AccessBarrier<decorators, BarrierSetT> Raw;

Copy link
Contributor

@shipilev shipilev Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks unnecessary.

public:
// Heap oop accesses. These accessors get resolved when
// IN_HEAP is set (e.g. when using the HeapAccess API), it is
@@ -99,29 +99,29 @@ inline oop ShenandoahBarrierSet::load_reference_barrier(oop obj) {
return obj;
}

template <DecoratorSet decorators, class T>
inline oop ShenandoahBarrierSet::load_reference_barrier(oop obj, T* load_addr) {
template <class T>
inline oop ShenandoahBarrierSet::load_reference_barrier(DecoratorSet decorators, oop obj, T* load_addr) {
if (obj == NULL) {
return NULL;
}

// Prevent resurrection of unreachable phantom (i.e. weak-native) references.
if (HasDecorator<decorators, ON_PHANTOM_OOP_REF>::value &&
if ((decorators & ON_PHANTOM_OOP_REF) != 0 &&
_heap->is_concurrent_weak_root_in_progress() &&
!_heap->marking_context()->is_marked(obj)) {
return NULL;
}

// Prevent resurrection of unreachable weak references.
if ((HasDecorator<decorators, ON_WEAK_OOP_REF>::value || HasDecorator<decorators, ON_UNKNOWN_OOP_REF>::value) &&
if ((decorators & ON_WEAK_OOP_REF) != 0 &&
_heap->is_concurrent_weak_root_in_progress() &&
!_heap->marking_context()->is_marked_strong(obj)) {
return NULL;
}

// Prevent resurrection of unreachable objects that are visited during
// concurrent class-unloading.
if (HasDecorator<decorators, AS_NO_KEEPALIVE>::value &&
if ((decorators & AS_NO_KEEPALIVE) != 0 &&
_heap->is_evacuation_in_progress() &&
!_heap->marking_context()->is_marked(obj)) {
return obj;
@@ -184,45 +184,64 @@ inline void ShenandoahBarrierSet::keep_alive_if_weak(DecoratorSet decorators, oo
}
}

template <DecoratorSet decorators>
inline void ShenandoahBarrierSet::keep_alive_if_weak(oop value) {
assert((decorators & ON_UNKNOWN_OOP_REF) == 0, "Reference strength must be known");
if (!HasDecorator<decorators, ON_STRONG_OOP_REF>::value &&
!HasDecorator<decorators, AS_NO_KEEPALIVE>::value) {
satb_enqueue(value);
}
template <typename T>
inline oop ShenandoahBarrierSet::oop_load(DecoratorSet decorators, T* addr) {
oop value = RawAccess<>::oop_load(addr);
value = load_reference_barrier(decorators, value, addr);
keep_alive_if_weak(decorators, value);
return value;
}

template <typename T>
inline oop ShenandoahBarrierSet::oop_cmpxchg(DecoratorSet decorators, T* addr, oop compare_value, oop new_value) {
iu_barrier(new_value);
oop res;
oop expected = compare_value;
do {
compare_value = expected;
res = RawAccess<>::oop_atomic_cmpxchg(addr, compare_value, new_value);
expected = res;
} while ((compare_value != expected) && (resolve_forwarded(compare_value) == resolve_forwarded(expected)));

// Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
// because it must be the previous value.
res = load_reference_barrier(decorators, res, reinterpret_cast<T*>(NULL));
satb_enqueue(res);
return res;
}

template <typename T>
inline oop ShenandoahBarrierSet::oop_xchg(DecoratorSet decorators, T* addr, oop new_value) {
iu_barrier(new_value);
oop previous = RawAccess<>::oop_atomic_xchg(addr, new_value);
// Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
// because it must be the previous value.
previous = load_reference_barrier<T>(decorators, previous, reinterpret_cast<T*>(NULL));
satb_enqueue(previous);
return previous;
}

template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_load_not_in_heap(T* addr) {
oop value = Raw::oop_load_not_in_heap(addr);
if (value != NULL) {
ShenandoahBarrierSet *const bs = ShenandoahBarrierSet::barrier_set();
value = bs->load_reference_barrier<decorators, T>(value, addr);
bs->keep_alive_if_weak<decorators>(value);
}
return value;
assert((decorators & ON_UNKNOWN_OOP_REF) == 0, "must be absent");
ShenandoahBarrierSet* const bs = ShenandoahBarrierSet::barrier_set();
return bs->oop_load(decorators, addr);
}

template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_load_in_heap(T* addr) {
oop value = Raw::oop_load_in_heap(addr);
ShenandoahBarrierSet *const bs = ShenandoahBarrierSet::barrier_set();
value = bs->load_reference_barrier<decorators, T>(value, addr);
bs->keep_alive_if_weak<decorators>(value);
return value;
assert((decorators & ON_UNKNOWN_OOP_REF) == 0, "must be absent");
ShenandoahBarrierSet* const bs = ShenandoahBarrierSet::barrier_set();
return bs->oop_load(decorators, addr);
}

template <DecoratorSet decorators, typename BarrierSetT>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_load_in_heap_at(oop base, ptrdiff_t offset) {
oop value = Raw::oop_load_in_heap_at(base, offset);
ShenandoahBarrierSet *const bs = ShenandoahBarrierSet::barrier_set();
ShenandoahBarrierSet* const bs = ShenandoahBarrierSet::barrier_set();
DecoratorSet resolved_decorators = AccessBarrierSupport::resolve_possibly_unknown_oop_ref_strength<decorators>(base, offset);
value = bs->load_reference_barrier<decorators>(value, AccessInternal::oop_field_addr<decorators>(base, offset));
bs->keep_alive_if_weak(resolved_decorators, value);
return value;
return bs->oop_load(resolved_decorators, AccessInternal::oop_field_addr<decorators>(base, offset));
}

template <DecoratorSet decorators, typename BarrierSetT>
@@ -254,59 +273,55 @@ inline void ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_st
template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_not_in_heap(T* addr, oop compare_value, oop new_value) {
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
bs->iu_barrier(new_value);

oop res;
oop expected = compare_value;
do {
compare_value = expected;
res = Raw::oop_atomic_cmpxchg(addr, compare_value, new_value);
expected = res;
} while ((compare_value != expected) && (resolve_forwarded(compare_value) == resolve_forwarded(expected)));

// Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
// because it must be the previous value.
res = ShenandoahBarrierSet::barrier_set()->load_reference_barrier<decorators, T>(res, NULL);
bs->satb_enqueue(res);
return res;
return bs->oop_cmpxchg(decorators, addr, compare_value, new_value);
}

template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_in_heap(T* addr, oop compare_value, oop new_value) {
return oop_atomic_cmpxchg_not_in_heap(addr, compare_value, new_value);
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
return bs->oop_cmpxchg(decorators, addr, compare_value, new_value);
}

template <DecoratorSet decorators, typename BarrierSetT>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_cmpxchg_in_heap_at(oop base, ptrdiff_t offset, oop compare_value, oop new_value) {
return oop_atomic_cmpxchg_in_heap(AccessInternal::oop_field_addr<decorators>(base, offset), compare_value, new_value);
assert((decorators & (ON_STRONG_OOP_REF | ON_UNKNOWN_OOP_REF)) != 0, "must be present");
assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
DecoratorSet resolved_decorators = AccessBarrierSupport::resolve_possibly_unknown_oop_ref_strength<decorators>(base, offset);
return bs->oop_cmpxchg(resolved_decorators, AccessInternal::oop_field_addr<decorators>(base, offset), compare_value, new_value);
}

template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_not_in_heap(T* addr, oop new_value) {
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
bs->iu_barrier(new_value);

oop previous = Raw::oop_atomic_xchg(addr, new_value);

// Note: We don't need a keep-alive-barrier here. We already enqueue any loaded reference for SATB anyway,
// because it must be the previous value.
previous = ShenandoahBarrierSet::barrier_set()->load_reference_barrier<decorators, T>(previous, NULL);
bs->satb_enqueue(previous);
return previous;
return bs->oop_xchg(decorators, addr, new_value);
}

template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap(T* addr, oop new_value) {
return oop_atomic_xchg_not_in_heap(addr, new_value);
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
return bs->oop_xchg(decorators, addr, new_value);
}

template <DecoratorSet decorators, typename BarrierSetT>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_atomic_xchg_in_heap_at(oop base, ptrdiff_t offset, oop new_value) {
return oop_atomic_xchg_in_heap(AccessInternal::oop_field_addr<decorators>(base, offset), new_value);
assert((decorators & ON_STRONG_OOP_REF) != 0, "must be present");
Copy link
Contributor

@shipilev shipilev Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be | ON_UNKNOWN_OOP_REF? We are doing resolve_possibly_unknown_oop_ref_strength later.

In fact, I am not so sure about asserting ON_STRONG_OOP_REF here and elsewhere as well. The future ON_WEAK_OOP_REF code should work fine here too? I think only asserting that ON_Uresolve_possibly_unknown_oop_ref_strengthNKNOWN_OOP_REF is not passed on bad paths is enough?

assert((decorators & AS_NO_KEEPALIVE) == 0, "must be absent");
ShenandoahBarrierSet* bs = ShenandoahBarrierSet::barrier_set();
DecoratorSet resolved_decorators = AccessBarrierSupport::resolve_possibly_unknown_oop_ref_strength<decorators>(base, offset);
return bs->oop_xchg(resolved_decorators, AccessInternal::oop_field_addr<decorators>(base, offset), new_value);
}

// Clone barrier support
@@ -68,17 +68,17 @@ JRT_LEAF(void, ShenandoahRuntime::shenandoah_clone_barrier(oopDesc* src))
JRT_END

JRT_LEAF(oopDesc*, ShenandoahRuntime::load_reference_barrier_weak(oopDesc * src, oop* load_addr))
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<ON_WEAK_OOP_REF, oop>(oop(src), load_addr);
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<oop>(ON_WEAK_OOP_REF, oop(src), load_addr);
JRT_END

JRT_LEAF(oopDesc*, ShenandoahRuntime::load_reference_barrier_weak_narrow(oopDesc * src, narrowOop* load_addr))
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<ON_WEAK_OOP_REF, narrowOop>(oop(src), load_addr);
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<narrowOop>(ON_WEAK_OOP_REF, oop(src), load_addr);
JRT_END

JRT_LEAF(oopDesc*, ShenandoahRuntime::load_reference_barrier_phantom(oopDesc * src, oop* load_addr))
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<ON_PHANTOM_OOP_REF, oop>(oop(src), load_addr);
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<oop>(ON_PHANTOM_OOP_REF, oop(src), load_addr);
JRT_END

JRT_LEAF(oopDesc*, ShenandoahRuntime::load_reference_barrier_phantom_narrow(oopDesc * src, narrowOop* load_addr))
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<ON_PHANTOM_OOP_REF, narrowOop>(oop(src), load_addr);
return (oopDesc*) ShenandoahBarrierSet::barrier_set()->load_reference_barrier<narrowOop>(ON_PHANTOM_OOP_REF, oop(src), load_addr);
JRT_END