Commit f4b41099 authored by Gabriel Charette's avatar Gabriel Charette Committed by Commit Bot

Introduce ConcurrentMarking::StopRequest API.

This was extracted from https://chromium-review.googlesource.com/c/v8/v8/+/924073/10
after it became clear that using COMPLETE_TASKS/PREEMPT_TASKS where
it should make sense to doesn't work in practice for now.

Experimental CLs which led to the above conclusion:
 - https://chromium-review.googlesource.com/c/v8/v8/+/924865
   (COMPLETE or CANCEL -- still broken)
 - https://chromium-review.googlesource.com/c/v8/v8/+/924866
   (CANCEL only, as before, works)
 - https://chromium-review.googlesource.com/c/v8/v8/+/924028
   (CANCEL and PREEMPT -- broken as well)

Introducing this unittested API allows to reduce the size
of the CLs causing hard-to-diagnose bots-only failures
and fix them individually follow-ups @

 1) https://chromium-review.googlesource.com/c/v8/v8/+/924029
 2) https://chromium-review.googlesource.com/c/v8/v8/+/924031
 3) https://chromium-review.googlesource.com/c/v8/v8/+/924030

Bug: chromium:812178
Change-Id: Icdac456e9f7874b0c4b321ccdb8898297dad7d73
Reviewed-on: https://chromium-review.googlesource.com/924867
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: 's avatarUlan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51353}
parent bd0b32f5
......@@ -480,6 +480,8 @@ void ConcurrentMarking::Run(int task_id, TaskState* task_state) {
marked_bytes);
if (task_state->interrupt_request.Value()) {
task_state->interrupt_condition.Wait(&task_state->lock);
} else if (task_state->preemption_request.Value()) {
break;
}
}
// The lock is also required to synchronize with worklist update after
......@@ -527,6 +529,7 @@ void ConcurrentMarking::ScheduleTasks() {
heap_->isolate()->PrintWithTimestamp(
"Scheduling concurrent marking task %d\n", i);
}
task_state_[i].preemption_request.SetValue(false);
task_state_[i].interrupt_request.SetValue(false);
is_pending_[i] = true;
++pending_task_count_;
......@@ -550,17 +553,11 @@ void ConcurrentMarking::RescheduleTasksIfNeeded() {
}
}
void ConcurrentMarking::WaitForTasks() {
void ConcurrentMarking::Stop(StopRequest stop_request) {
if (!FLAG_concurrent_marking) return;
base::LockGuard<base::Mutex> guard(&pending_lock_);
while (pending_task_count_ > 0) {
pending_condition_.Wait(&pending_lock_);
}
}
void ConcurrentMarking::EnsureCompleted() {
if (!FLAG_concurrent_marking) return;
base::LockGuard<base::Mutex> guard(&pending_lock_);
if (stop_request != StopRequest::COMPLETE_TASKS_FOR_TESTING) {
CancelableTaskManager* task_manager =
heap_->isolate()->cancelable_task_manager();
for (int i = 1; i <= task_count_; i++) {
......@@ -569,6 +566,9 @@ void ConcurrentMarking::EnsureCompleted() {
CancelableTaskManager::kTaskAborted) {
is_pending_[i] = false;
--pending_task_count_;
} else if (stop_request == StopRequest::PREEMPT_TASKS) {
task_state_[i].preemption_request.SetValue(true);
}
}
}
}
......
......@@ -40,6 +40,17 @@ class ConcurrentMarking {
ConcurrentMarking* concurrent_marking_;
};
enum class StopRequest {
// Preempt ongoing tasks ASAP (and cancel unstarted tasks).
PREEMPT_TASKS,
// Wait for ongoing tasks to complete (and cancels unstarted tasks).
COMPLETE_ONGOING_TASKS,
// Wait for all scheduled tasks to complete (only use this in tests that
// control the full stack -- otherwise tasks cancelled by the platform can
// make this call hang).
COMPLETE_TASKS_FOR_TESTING,
};
static constexpr int kMaxTasks = 4;
using MarkingWorklist = Worklist<HeapObject*, 64 /* segment size */>;
......@@ -47,9 +58,12 @@ class ConcurrentMarking {
MarkingWorklist* bailout, MarkingWorklist* on_hold,
WeakObjects* weak_objects);
// Schedules asynchronous tasks to perform concurrent marking. Objects in the
// heap should not be moved while these are active (can be stopped safely via
// Stop() or PauseScope).
void ScheduleTasks();
void WaitForTasks();
void EnsureCompleted();
void Stop(StopRequest stop_request);
void RescheduleTasksIfNeeded();
// Flushes the local live bytes into the given marking state.
void FlushLiveBytes(MajorNonAtomicMarkingState* marking_state);
......@@ -63,6 +77,10 @@ class ConcurrentMarking {
private:
struct TaskState {
// The main thread sets this flag to true when it wants the concurrent
// marker to give up the worker thread.
base::AtomicValue<bool> preemption_request;
// When the concurrent marking task has this lock, then objects in the
// heap are guaranteed to not move.
base::Mutex lock;
......
......@@ -16,7 +16,6 @@
#include "src/global-handles.h"
#include "src/heap/array-buffer-collector.h"
#include "src/heap/array-buffer-tracker-inl.h"
#include "src/heap/concurrent-marking.h"
#include "src/heap/gc-tracer.h"
#include "src/heap/incremental-marking.h"
#include "src/heap/invalidated-slots-inl.h"
......@@ -867,7 +866,9 @@ void MarkCompactCollector::Prepare() {
if (was_marked_incrementally_ && heap_->ShouldAbortIncrementalMarking()) {
heap()->incremental_marking()->Stop();
heap()->incremental_marking()->AbortBlackAllocation();
FinishConcurrentMarking();
// TODO(gab): PREEMPT_TASKS here in a follow-up CL.
FinishConcurrentMarking(
ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
heap()->incremental_marking()->Deactivate();
ClearMarkbits();
AbortWeakCollections();
......@@ -903,9 +904,10 @@ void MarkCompactCollector::Prepare() {
#endif
}
void MarkCompactCollector::FinishConcurrentMarking() {
void MarkCompactCollector::FinishConcurrentMarking(
ConcurrentMarking::StopRequest stop_request) {
if (FLAG_concurrent_marking) {
heap()->concurrent_marking()->EnsureCompleted();
heap()->concurrent_marking()->Stop(stop_request);
heap()->concurrent_marking()->FlushLiveBytes(non_atomic_marking_state());
}
}
......@@ -2358,7 +2360,8 @@ void MarkCompactCollector::MarkLiveObjects() {
}
ProcessMarkingWorklist();
FinishConcurrentMarking();
FinishConcurrentMarking(
ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
ProcessMarkingWorklist();
}
......
......@@ -8,6 +8,7 @@
#include <deque>
#include <vector>
#include "src/heap/concurrent-marking.h"
#include "src/heap/marking.h"
#include "src/heap/objects-visiting.h"
#include "src/heap/spaces.h"
......@@ -649,7 +650,9 @@ class MarkCompactCollector final : public MarkCompactCollectorBase {
// choosing spaces to compact.
void Prepare();
void FinishConcurrentMarking();
// Stop concurrent marking (either by preempting it right away or waiting for
// it to complete as requested by |stop_request|).
void FinishConcurrentMarking(ConcurrentMarking::StopRequest stop_request);
bool StartCompaction();
......
......@@ -44,8 +44,8 @@ TEST(ConcurrentMarking) {
new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
PublishSegment(&shared, heap->undefined_value());
concurrent_marking->ScheduleTasks();
concurrent_marking->WaitForTasks();
concurrent_marking->EnsureCompleted();
concurrent_marking->Stop(
ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
delete concurrent_marking;
}
......@@ -66,12 +66,39 @@ TEST(ConcurrentMarkingReschedule) {
new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
PublishSegment(&shared, heap->undefined_value());
concurrent_marking->ScheduleTasks();
concurrent_marking->WaitForTasks();
concurrent_marking->EnsureCompleted();
concurrent_marking->Stop(
ConcurrentMarking::StopRequest::COMPLETE_ONGOING_TASKS);
PublishSegment(&shared, heap->undefined_value());
concurrent_marking->RescheduleTasksIfNeeded();
concurrent_marking->WaitForTasks();
concurrent_marking->EnsureCompleted();
concurrent_marking->Stop(
ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
delete concurrent_marking;
}
TEST(ConcurrentMarkingPreemptAndReschedule) {
if (!i::FLAG_concurrent_marking) return;
CcTest::InitializeVM();
Heap* heap = CcTest::heap();
CcTest::CollectAllGarbage();
if (!heap->incremental_marking()->IsStopped()) return;
MarkCompactCollector* collector = CcTest::heap()->mark_compact_collector();
if (collector->sweeping_in_progress()) {
collector->EnsureSweepingCompleted();
}
ConcurrentMarking::MarkingWorklist shared, bailout, on_hold;
WeakObjects weak_objects;
ConcurrentMarking* concurrent_marking =
new ConcurrentMarking(heap, &shared, &bailout, &on_hold, &weak_objects);
for (int i = 0; i < 5000; i++)
PublishSegment(&shared, heap->undefined_value());
concurrent_marking->ScheduleTasks();
concurrent_marking->Stop(ConcurrentMarking::StopRequest::PREEMPT_TASKS);
for (int i = 0; i < 5000; i++)
PublishSegment(&shared, heap->undefined_value());
concurrent_marking->RescheduleTasksIfNeeded();
concurrent_marking->Stop(
ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
delete concurrent_marking;
}
......@@ -85,8 +112,8 @@ TEST(ConcurrentMarkingMarkedBytes) {
CcTest::CollectAllGarbage();
if (!heap->incremental_marking()->IsStopped()) return;
heap::SimulateIncrementalMarking(heap, false);
heap->concurrent_marking()->WaitForTasks();
heap->concurrent_marking()->EnsureCompleted();
heap->concurrent_marking()->Stop(
ConcurrentMarking::StopRequest::COMPLETE_TASKS_FOR_TESTING);
CHECK_GE(heap->concurrent_marking()->TotalMarkedBytes(), root->Size());
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment