Skip to content

Commit

Permalink
deps: backport bca8409 from upstream V8
Browse files Browse the repository at this point in the history
Original commit message:

  Make CancelableTask ids unique
  They were only limited to 32 bit when using the internal Hashmap. Since
  this has changed alreay some time ago, we can switch to 64 bit ids and
  check that we never overflow.

  Bug:
  Change-Id: Ia6c6d02d6b5e555c6941185a79427dc4aa2a1d62
  Reviewed-on: https://chromium-review.googlesource.com/598229
  Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#47085}

PR-URL: nodejs#14001
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
  • Loading branch information
Matt Loring authored and targos committed Sep 14, 2017
1 parent fd4a6da commit 955526d
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 19 deletions.
13 changes: 6 additions & 7 deletions deps/v8/src/cancelable-task.cc
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,17 @@ Cancelable::~Cancelable() {
CancelableTaskManager::CancelableTaskManager()
: task_id_counter_(0), canceled_(false) {}

uint32_t CancelableTaskManager::Register(Cancelable* task) {
CancelableTaskManager::Id CancelableTaskManager::Register(Cancelable* task) {
base::LockGuard<base::Mutex> guard(&mutex_);
uint32_t id = ++task_id_counter_;
// The loop below is just used when task_id_counter_ overflows.
while (cancelable_tasks_.count(id) > 0) ++id;
CancelableTaskManager::Id id = ++task_id_counter_;
// Id overflows are not supported.
CHECK_NE(0, id);
CHECK(!canceled_);
cancelable_tasks_[id] = task;
return id;
}


void CancelableTaskManager::RemoveFinishedTask(uint32_t id) {
void CancelableTaskManager::RemoveFinishedTask(CancelableTaskManager::Id id) {
base::LockGuard<base::Mutex> guard(&mutex_);
size_t removed = cancelable_tasks_.erase(id);
USE(removed);
Expand All @@ -49,7 +48,7 @@ void CancelableTaskManager::RemoveFinishedTask(uint32_t id) {
}

CancelableTaskManager::TryAbortResult CancelableTaskManager::TryAbort(
uint32_t id) {
CancelableTaskManager::Id id) {
base::LockGuard<base::Mutex> guard(&mutex_);
auto entry = cancelable_tasks_.find(id);
if (entry != cancelable_tasks_.end()) {
Expand Down
18 changes: 10 additions & 8 deletions deps/v8/src/cancelable-task.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
#ifndef V8_CANCELABLE_TASK_H_
#define V8_CANCELABLE_TASK_H_

#include <map>
#include <unordered_map>

#include "include/v8-platform.h"
#include "src/base/atomic-utils.h"
Expand All @@ -24,12 +24,14 @@ class Isolate;
// from any fore- and background task/thread.
class V8_EXPORT_PRIVATE CancelableTaskManager {
public:
using Id = uint64_t;

CancelableTaskManager();

// Registers a new cancelable {task}. Returns the unique {id} of the task that
// can be used to try to abort a task by calling {Abort}.
// Must not be called after CancelAndWait.
uint32_t Register(Cancelable* task);
Id Register(Cancelable* task);

// Try to abort running a task identified by {id}. The possible outcomes are:
// (1) The task is already finished running or was canceled before and
Expand All @@ -39,7 +41,7 @@ class V8_EXPORT_PRIVATE CancelableTaskManager {
// removed.
//
enum TryAbortResult { kTaskRemoved, kTaskRunning, kTaskAborted };
TryAbortResult TryAbort(uint32_t id);
TryAbortResult TryAbort(Id id);

// Cancels all remaining registered tasks and waits for tasks that are
// already running. This disallows subsequent Register calls.
Expand All @@ -59,13 +61,13 @@ class V8_EXPORT_PRIVATE CancelableTaskManager {
private:
// Only called by {Cancelable} destructor. The task is done with executing,
// but needs to be removed.
void RemoveFinishedTask(uint32_t id);
void RemoveFinishedTask(Id id);

// To mitigate the ABA problem, the api refers to tasks through an id.
uint32_t task_id_counter_;
Id task_id_counter_;

// A set of cancelable tasks that are currently registered.
std::map<uint32_t, Cancelable*> cancelable_tasks_;
std::unordered_map<Id, Cancelable*> cancelable_tasks_;

// Mutex and condition variable enabling concurrent register and removing, as
// well as waiting for background tasks on {CancelAndWait}.
Expand All @@ -89,7 +91,7 @@ class V8_EXPORT_PRIVATE Cancelable {
// a platform. This step transfers ownership to the platform, which destroys
// the task after running it. Since the exact time is not known, we cannot
// access the object after handing it to a platform.
uint32_t id() { return id_; }
CancelableTaskManager::Id id() { return id_; }

protected:
bool TryRun() { return status_.TrySetValue(kWaiting, kRunning); }
Expand Down Expand Up @@ -120,7 +122,7 @@ class V8_EXPORT_PRIVATE Cancelable {

CancelableTaskManager* parent_;
base::AtomicValue<Status> status_;
uint32_t id_;
CancelableTaskManager::Id id_;

// The counter is incremented for failing tries to cancel a task. This can be
// used by the task itself as an indication how often external entities tried
Expand Down
3 changes: 2 additions & 1 deletion deps/v8/src/heap/item-parallel-job.h
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ class ItemParallelJob {
const size_t num_tasks = tasks_.size();
const size_t num_items = items_.size();
const size_t items_per_task = (num_items + num_tasks - 1) / num_tasks;
uint32_t* task_ids = new uint32_t[num_tasks];
CancelableTaskManager::Id* task_ids =
new CancelableTaskManager::Id[num_tasks];
size_t start_index = 0;
Task* main_task = nullptr;
Task* task = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/src/heap/mark-compact.h
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ class MarkCompactCollector final : public MarkCompactCollectorBase {

Heap* const heap_;
int num_tasks_;
uint32_t task_ids_[kMaxSweeperTasks];
CancelableTaskManager::Id task_ids_[kMaxSweeperTasks];
base::Semaphore pending_sweeper_tasks_semaphore_;
base::Mutex mutex_;
SweptList swept_list_[kAllocationSpaces];
Expand Down
4 changes: 2 additions & 2 deletions deps/v8/test/unittests/cancelable-tasks-unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ TEST(CancelableTask, RemoveBeforeCancelAndWait) {
ResultType result1 = 0;
TestTask* task1 = new TestTask(&manager, &result1, TestTask::kCheckNotRun);
ThreadedRunner runner1(task1);
uint32_t id = task1->id();
CancelableTaskManager::Id id = task1->id();
EXPECT_EQ(id, 1u);
EXPECT_TRUE(manager.TryAbort(id));
runner1.Start();
Expand All @@ -195,7 +195,7 @@ TEST(CancelableTask, RemoveAfterCancelAndWait) {
ResultType result1 = 0;
TestTask* task1 = new TestTask(&manager, &result1);
ThreadedRunner runner1(task1);
uint32_t id = task1->id();
CancelableTaskManager::Id id = task1->id();
EXPECT_EQ(id, 1u);
runner1.Start();
runner1.Join();
Expand Down

0 comments on commit 955526d

Please sign in to comment.