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 1 commit
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
@@ -132,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
@@ -224,13 +224,15 @@ inline oop ShenandoahBarrierSet::oop_xchg(DecoratorSet decorators, T* addr, oop
template <DecoratorSet decorators, typename BarrierSetT>
template <typename T>
inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_load_not_in_heap(T* addr) {
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) {
assert((decorators & ON_UNKNOWN_OOP_REF) == 0, "must be absent");
ShenandoahBarrierSet* const bs = ShenandoahBarrierSet::barrier_set();
return bs->oop_load(decorators, addr);
}
@@ -271,19 +273,25 @@ 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();
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) {
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) {
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);
@@ -292,19 +300,25 @@ inline oop ShenandoahBarrierSet::AccessBarrier<decorators, BarrierSetT>::oop_ato
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();
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) {
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) {
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);