Avoid invalidating kInstrumented via suspension
Make sure we no longer trust kInstrumented after executing allocation
code that may suspend.
Don't just switch to the new normal allocator if we were originally
asked for a nonmoving allocation.
Document the convention we need to enforce.
Test: Build and boot AOSP.
Bug: 187958881
Change-Id: Ife722082e87f85e907bfed3f5371865a5642157f
diff --git a/runtime/alloc_instrumentation.md b/runtime/alloc_instrumentation.md
new file mode 100644
index 0000000..513bbe3
--- /dev/null
+++ b/runtime/alloc_instrumentation.md
@@ -0,0 +1,33 @@
+In order to buy some performance on the common, uninstrumented, fast path, we replace repeated
+checks for both allocation instrumentation and allocator changes by a single function table
+dispatch, and templatized allocation code that can be used to generate either instrumented
+or uninstrumented versions of allocation routines.
+
+When we call an allocation routine, we always indirect through a thread-local function table that
+either points to instrumented or uninstrumented allocation routines. The instrumented code has a
+`kInstrumented` = true template argument (or `kIsInstrumented` in some places), the uninstrumented
+code has `kInstrumented` = false.
+
+The function table is thread-local. There appears to be no logical necessity for that; it just
+makes it easier to access from compiled Java code.
+
+- The function table is switched out by `InstrumentQuickAllocEntryPoints[Locked]`, and a
+corresponding `UninstrumentQuickAlloc`... function.
+
+- These in turn are called by `SetStatsEnabled()`, `SetAllocationListener()`, et al, which
+require the mutator lock is not held.
+
+- With a started runtime, `SetEntrypointsInstrumented()` calls `ScopedSupendAll(`) before updating
+ the function table.
+
+Mutual exclusion in the dispatch table is thus ensured by the fact that it is only updated while
+all other threads are suspended, and is only accessed with the mutator lock logically held,
+which inhibits suspension.
+
+To ensure correctness, we thus must:
+
+1. Suspend all threads when swapping out the dispatch table, and
+2. Make sure that we hold the mutator lock when accessing it.
+3. Not trust kInstrumented once we've given up the mutator lock, since it could have changed in the
+ interim.
+
diff --git a/runtime/gc/heap-inl.h b/runtime/gc/heap-inl.h
index 4cfbe2d..3fb2f45 100644
--- a/runtime/gc/heap-inl.h
+++ b/runtime/gc/heap-inl.h
@@ -66,7 +66,7 @@
self->PoisonObjectPointers();
}
auto pre_object_allocated = [&]() REQUIRES_SHARED(Locks::mutator_lock_)
- REQUIRES(!Roles::uninterruptible_) {
+ REQUIRES(!Roles::uninterruptible_ /* only suspends if kInstrumented */) {
if constexpr (kInstrumented) {
AllocationListener* l = alloc_listener_.load(std::memory_order_seq_cst);
if (UNLIKELY(l != nullptr) && UNLIKELY(l->HasPreAlloc())) {
@@ -88,14 +88,13 @@
// non-TLAB object allocations. Only set for non-thread-local allocation,
size_t bytes_tl_bulk_allocated = 0u;
// Do the initial pre-alloc
+ // TODO: Consider what happens if the allocator is switched while suspended here.
pre_object_allocated();
- ScopedAssertNoThreadSuspension ants("Called PreObjectAllocated, no suspend until alloc");
// Need to check that we aren't the large object allocator since the large object allocation
// code path includes this function. If we didn't check we would have an infinite loop.
if (kCheckLargeObject && UNLIKELY(ShouldAllocLargeObject(klass, byte_count))) {
// AllocLargeObject can suspend and will recall PreObjectAllocated if needed.
- ScopedAllowThreadSuspension ats;
obj = AllocLargeObject<kInstrumented, PreFenceVisitor>(self, &klass, byte_count,
pre_fence_visitor);
if (obj != nullptr) {
@@ -107,8 +106,12 @@
// If the large object allocation failed, try to use the normal spaces (main space,
// non moving space). This can happen if there is significant virtual address space
// fragmentation.
- pre_object_allocated();
+ // kInstrumented may be out of date, so recurse without large object checking, rather than
+ // continue.
+ return AllocObjectWithAllocator</*kInstrumented=*/ true, /*kCheckLargeObject=*/ false>
+ (self, klass, byte_count, GetUpdatedAllocator(allocator), pre_fence_visitor);
}
+ ScopedAssertNoThreadSuspension ants("Called PreObjectAllocated, no suspend until alloc");
if (IsTLABAllocator(allocator)) {
byte_count = RoundUp(byte_count, space::BumpPointerSpace::kAlignment);
}
@@ -140,9 +143,9 @@
obj = TryToAllocate<kInstrumented, false>(self, allocator, byte_count, &bytes_allocated,
&usable_size, &bytes_tl_bulk_allocated);
if (UNLIKELY(obj == nullptr)) {
- // AllocateInternalWithGc can cause thread suspension, if someone instruments the
- // entrypoints or changes the allocator in a suspend point here, we need to retry the
- // allocation. It will send the pre-alloc event again.
+ // AllocateInternalWithGc internally re-allows, and can cause, thread suspension, if
+ // someone instruments the entrypoints or changes the allocator in a suspend point here,
+ // we need to retry the allocation. It will send the pre-alloc event again.
obj = AllocateInternalWithGc(self,
allocator,
kInstrumented,
@@ -159,13 +162,15 @@
ScopedAllowThreadSuspension ats;
// AllocObject will pick up the new allocator type, and instrumented as true is the safe
// default.
- return AllocObject</*kInstrumented=*/true>(self,
- klass,
- byte_count,
- pre_fence_visitor);
+ return AllocObjectWithAllocator</*kInstrumented=*/true>(self,
+ klass,
+ byte_count,
+ GetUpdatedAllocator(allocator),
+ pre_fence_visitor);
}
return nullptr;
}
+ // Non-null result implies neither instrumentation nor allocator changed.
}
DCHECK_GT(bytes_allocated, 0u);
DCHECK_GT(usable_size, 0u);
diff --git a/runtime/gc/heap.h b/runtime/gc/heap.h
index d21aafa..5d0ba8f 100644
--- a/runtime/gc/heap.h
+++ b/runtime/gc/heap.h
@@ -287,6 +287,11 @@
return current_non_moving_allocator_;
}
+ AllocatorType GetUpdatedAllocator(AllocatorType old_allocator) {
+ return (old_allocator == kAllocatorTypeNonMoving) ?
+ GetCurrentNonMovingAllocator() : GetCurrentAllocator();
+ }
+
// Visit all of the live objects in the heap.
template <typename Visitor>
ALWAYS_INLINE void VisitObjects(Visitor&& visitor)
@@ -1030,8 +1035,10 @@
REQUIRES(!*gc_complete_lock_, !*pending_task_lock_,
!*backtrace_lock_, !process_state_update_lock_);
- // Handles Allocate()'s slow allocation path with GC involved after
- // an initial allocation attempt failed.
+ // Handles Allocate()'s slow allocation path with GC involved after an initial allocation
+ // attempt failed.
+ // Called with thread suspension disallowed, but re-enables it, and may suspend, internally.
+ // Returns null if instrumentation or the allocator changed.
mirror::Object* AllocateInternalWithGc(Thread* self,
AllocatorType allocator,
bool instrumented,