Skip to content

Commit

Permalink
[task]: Remove TaskDestructionDetector
Browse files Browse the repository at this point in the history 
delayed_task_handle_.IsValid() is sufficient and simpler than an
observer pattern.

Bug: 1262205
Change-Id: Ibddeaa47298772508a2ebfdf5df7ff9e462f1878
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3378413
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1002873}
  • Loading branch information
Etienne Pierre-doray authored and Chromium LUCI CQ committed May 12, 2022
1 parent 251e32e commit 9c3d17d
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 135 deletions.
8 changes: 4 additions & 4 deletions base/task/sequence_manager/sequence_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,10 @@ SequenceManagerImpl::SelectNextTaskImpl(SelectTaskOption option) {
*main_thread_only().task_execution_stack.rbegin();
NotifyWillProcessTask(&executing_task, &lazy_now);

// Maybe invalidate the delayed task handle. |pending_task| is guaranteed to
// be valid here (not canceled).
executing_task.pending_task.WillRunTask();

return SelectedTask(
executing_task.pending_task,
executing_task.task_queue->task_execution_trace_logger());
Expand Down Expand Up @@ -880,10 +884,6 @@ void SequenceManagerImpl::NotifyWillProcessTask(ExecutingTask* executing_task,
if (recording_policy == TimeRecordingPolicy::DoRecord)
executing_task->task_timing.RecordTaskStart(time_before_task);

// Maybe invalidate the delayed task handle. |pending_task| is guaranteed to
// be valid here (not canceled).
executing_task->pending_task.WillRunTask();

if (!executing_task->task_queue->GetShouldNotifyObservers())
return;

Expand Down
123 changes: 29 additions & 94 deletions base/timer/timer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,41 +37,13 @@ bool g_is_always_abandon_scheduled_task_enabled =

} // namespace

// TaskDestructionDetector's role is to detect when the scheduled task is
// deleted without being executed. It can be disabled when the timer no longer
// wants to be notified.
class TaskDestructionDetector {
public:
explicit TaskDestructionDetector(TimerBase* timer) : timer_(timer) {}

TaskDestructionDetector(const TaskDestructionDetector&) = delete;
TaskDestructionDetector& operator=(const TaskDestructionDetector&) = delete;

~TaskDestructionDetector() {
// If this instance is getting destroyed before it was disabled, notify the
// timer.
if (timer_)
timer_->OnTaskDestroyed();
}

// Disables this instance so that the timer is no longer notified in the
// destructor.
void Disable() { timer_ = nullptr; }

private:
// `timer_` is not a raw_ptr<...> for performance reasons (based on analysis
// of sampling profiler data and tab_search:top100:2020).
RAW_PTR_EXCLUSION TimerBase* timer_;
};

// static
void TimerBase::InitializeFeatures() {
g_is_always_abandon_scheduled_task_enabled =
FeatureList::IsEnabled(kAlwaysAbandonScheduledTask);
}

TimerBase::TimerBase(const Location& posted_from)
: posted_from_(posted_from), task_destruction_detector_(nullptr) {
TimerBase::TimerBase(const Location& posted_from) : posted_from_(posted_from) {
// It is safe for the timer to be created on a different thread/sequence than
// the one from which the timer APIs are called. The first call to the
// checker's CalledOnValidSequence() method will re-bind the checker, and
Expand All @@ -86,7 +58,20 @@ TimerBase::~TimerBase() {

bool TimerBase::IsRunning() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return is_running_;

// When the `kAlwaysAbandonScheduledTask` feature is enabled, checking
// `delayed_task_handle_.IsValid()` is sufficient to determine if the
// timer is running. When the feature is disabled, the delayed task
// is not abandoned when the timer is stopped and the handle remains
// valid, so it's necessary to also check `is_running_` (set to false
// from `Stop()`).
//
// TODO(crbug.com/1262205): Remove the `is_running_` check once the
// "AlwaysAbandonScheduledTask" feature is launched.
if (!is_running_)
return false;

return delayed_task_handle_.IsValid();
}

void TimerBase::SetTaskRunner(scoped_refptr<SequencedTaskRunner> task_runner) {
Expand All @@ -100,23 +85,6 @@ scoped_refptr<SequencedTaskRunner> TimerBase::GetTaskRunner() {
return task_runner_ ? task_runner_ : SequencedTaskRunnerHandle::Get();
}

void TimerBase::OnTaskDestroyed() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

DCHECK(task_destruction_detector_);
task_destruction_detector_ = nullptr;

delayed_task_handle_.CancelTask();
is_running_ = false;

// It's safe to destroy or restart Timer on another sequence after it has been
// stopped.
DETACH_FROM_SEQUENCE(sequence_checker_);

OnStop();
// No more member accesses here: |this| could be deleted after OnStop() call.
}

void TimerBase::Stop() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

Expand All @@ -130,13 +98,8 @@ void TimerBase::Stop() {
void TimerBase::AbandonScheduledTask() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

if (task_destruction_detector_) {
task_destruction_detector_->Disable();
task_destruction_detector_ = nullptr;

DCHECK(delayed_task_handle_.IsValid());
if (delayed_task_handle_.IsValid())
delayed_task_handle_.CancelTask();
}

// It's safe to destroy or restart Timer on another sequence after the task is
// abandoned.
Expand Down Expand Up @@ -189,7 +152,7 @@ void DelayTimerBase::Reset() {

if (!g_is_always_abandon_scheduled_task_enabled) {
// If there's no pending task, start one up and return.
if (!task_destruction_detector_) {
if (!delayed_task_handle_.IsValid()) {
ScheduleNewTask(delay_);
return;
}
Expand Down Expand Up @@ -228,11 +191,8 @@ void DelayTimerBase::Stop() {

void DelayTimerBase::ScheduleNewTask(TimeDelta delay) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!task_destruction_detector_);
DCHECK(!delayed_task_handle_.IsValid());
is_running_ = true;
auto task_destruction_detector =
std::make_unique<TaskDestructionDetector>(this);
task_destruction_detector_ = task_destruction_detector.get();

// Ignore negative deltas.
// TODO(pmonette): Fix callers providing negative deltas and ban passing them.
Expand All @@ -241,8 +201,7 @@ void DelayTimerBase::ScheduleNewTask(TimeDelta delay) {

delayed_task_handle_ = GetTaskRunner()->PostCancelableDelayedTask(
base::subtle::PostDelayedTaskPassKey(), posted_from_,
BindOnce(&DelayTimerBase::OnScheduledTaskInvoked, Unretained(this),
std::move(task_destruction_detector)),
BindOnce(&DelayTimerBase::OnScheduledTaskInvoked, Unretained(this)),
delay);
scheduled_run_time_ = desired_run_time_ = Now() + delay;
}
Expand All @@ -252,16 +211,9 @@ TimeTicks DelayTimerBase::Now() const {
return tick_clock_ ? tick_clock_->NowTicks() : TimeTicks::Now();
}

void DelayTimerBase::OnScheduledTaskInvoked(
std::unique_ptr<TaskDestructionDetector> task_destruction_detector) {
void DelayTimerBase::OnScheduledTaskInvoked() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!delayed_task_handle_.IsValid());

// The scheduled task is currently running so its destruction detector is no
// longer needed.
task_destruction_detector->Disable();
task_destruction_detector_ = nullptr;
task_destruction_detector.reset();
DCHECK(!delayed_task_handle_.IsValid()) << posted_from_.ToString();

// The timer may have been stopped.
if (!is_running_)
Expand Down Expand Up @@ -411,6 +363,7 @@ void DeadlineTimer::Start(const Location& posted_from,
OnceClosure user_task,
ExactDeadline exact) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!delayed_task_handle_.IsValid());
user_task_ = std::move(user_task);
posted_from_ = posted_from;
subtle::DelayPolicy delay_policy =
Expand All @@ -426,44 +379,26 @@ void DeadlineTimer::OnStop() {
// |user_task_|.
}

void DeadlineTimer::RunUserTask() {
// Make a local copy of the task to run. The Stop method will reset the
// |user_task_| member.
OnceClosure task = std::move(user_task_);
Stop();
std::move(task).Run();
// No more member accesses here: |this| could be deleted at this point.
}

void DeadlineTimer::ScheduleNewTask(TimeTicks deadline,
subtle::DelayPolicy delay_policy) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!task_destruction_detector_);
is_running_ = true;
auto task_destruction_detector =
std::make_unique<internal::TaskDestructionDetector>(this);
task_destruction_detector_ = task_destruction_detector.get();

delayed_task_handle_ = GetTaskRunner()->PostCancelableDelayedTaskAt(
base::subtle::PostDelayedTaskPassKey(), posted_from_,
BindOnce(&DeadlineTimer::OnScheduledTaskInvoked, Unretained(this),
std::move(task_destruction_detector)),
BindOnce(&DeadlineTimer::OnScheduledTaskInvoked, Unretained(this)),
deadline, delay_policy);
}

void DeadlineTimer::OnScheduledTaskInvoked(
std::unique_ptr<internal::TaskDestructionDetector>
task_destruction_detector) {
void DeadlineTimer::OnScheduledTaskInvoked() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(!delayed_task_handle_.IsValid());

// The scheduled task is currently running so its destruction detector is no
// longer needed.
task_destruction_detector->Disable();
task_destruction_detector_ = nullptr;
task_destruction_detector.reset();

RunUserTask();
// Make a local copy of the task to run. The Stop method will reset the
// |user_task_| member.
OnceClosure task = std::move(user_task_);
Stop();
std::move(task).Run();
// No more member accesses here: |this| could be deleted at this point.
}

Expand Down
35 changes: 7 additions & 28 deletions base/timer/timer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,6 @@ using ExactDeadline = base::StrongAlias<class ExactDeadlineTag, bool>;

namespace internal {

class TaskDestructionDetector;

// This class wraps logic shared by all timers.
class BASE_EXPORT TimerBase {
public:
Expand Down Expand Up @@ -113,11 +111,10 @@ class BASE_EXPORT TimerBase {
// Constructs a timer. Start must be called later to set task info.
explicit TimerBase(const Location& posted_from = Location());

virtual void RunUserTask() = 0;
virtual void OnStop() = 0;

// Cancels the scheduled task and abandon it so that it no longer refers back
// to this object.
// Disables the scheduled task and abandons it so that it no longer refers
// back to this object.
void AbandonScheduledTask();

// Returns the task runner on which the task should be scheduled. If the
Expand All @@ -137,25 +134,12 @@ class BASE_EXPORT TimerBase {
// Location in user code.
Location posted_from_ GUARDED_BY_CONTEXT(sequence_checker_);

// Detects when the scheduled task is deleted before being executed. Null when
// there is no scheduled task.
// `task_destruction_detector_` is not a raw_ptr<...> for performance reasons
// (based on analysis of sampling profiler data).
RAW_PTR_EXCLUSION TaskDestructionDetector* task_destruction_detector_
GUARDED_BY_CONTEXT(sequence_checker_);

// If true, |user_task_| is scheduled to run sometime in the future.
// TODO(1262205): Remove once kAlwaysAbandonScheduledTask is gone.
bool is_running_ GUARDED_BY_CONTEXT(sequence_checker_) = false;

// The handle to the posted delayed task.
DelayedTaskHandle delayed_task_handle_ GUARDED_BY_CONTEXT(sequence_checker_);

private:
friend class TaskDestructionDetector;

// Indicates that the scheduled task was destroyed from inside the queue.
// Stops the timer if it was running.
void OnTaskDestroyed();
};

//-----------------------------------------------------------------------------
Expand Down Expand Up @@ -200,6 +184,8 @@ class BASE_EXPORT DelayTimerBase : public TimerBase {
TimeDelta delay,
const TickClock* tick_clock = nullptr);

virtual void RunUserTask() = 0;

// Schedules |OnScheduledTaskInvoked()| to run on the current sequence with
// the given |delay|. |scheduled_run_time_| and |desired_run_time_| are reset
// to Now() + delay.
Expand All @@ -217,10 +203,7 @@ class BASE_EXPORT DelayTimerBase : public TimerBase {

// Called when the scheduled task is invoked. Will run the |user_task| if the
// timer is still running and |desired_run_time_| was reached.
// |task_destruction_detector| is owned by the callback to detect when the
// scheduled task is deleted before being executed.
void OnScheduledTaskInvoked(
std::unique_ptr<TaskDestructionDetector> task_destruction_detector);
void OnScheduledTaskInvoked();

// Delay requested by user.
TimeDelta delay_ GUARDED_BY_CONTEXT(sequence_checker_);
Expand Down Expand Up @@ -457,18 +440,14 @@ class BASE_EXPORT DeadlineTimer : public internal::TimerBase {

protected:
void OnStop() override;
void RunUserTask() override;

// Schedules |OnScheduledTaskInvoked()| to run on the current sequence at
// the given |deadline|.
void ScheduleNewTask(TimeTicks deadline, subtle::DelayPolicy delay_policy);

private:
// Called when the scheduled task is invoked to run the |user_task|.
// |task_destruction_detector| is owned by the callback to detect when the
// scheduled task is deleted before being executed.
void OnScheduledTaskInvoked(std::unique_ptr<internal::TaskDestructionDetector>
task_destruction_detector);
void OnScheduledTaskInvoked();

OnceClosure user_task_;
};
Expand Down

0 comments on commit 9c3d17d

Please sign in to comment.