Skip to content

Rework the ThreadPlanStackMap class so we can move ThreadPlanStacks #3172

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Aug 17, 2021
Merged

Rework the ThreadPlanStackMap class so we can move ThreadPlanStacks #3172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0f9c8a4
Rework the ThreadPlanStackMap class so we can move ThreadPlanStacks
jimingham Aug 12, 2021
7ce93f8
Add a few auto's and note the detached stack has no nullptr's.
jimingham Aug 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 71 additions & 18 deletions lldb/include/lldb/Target/ThreadPlanStack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ class ThreadPlanStack {
/// generated.
void ClearThreadCache();

bool IsTID(lldb::tid_t tid);
bool IsTID(lldb::tid_t tid) {
return GetTID() == tid;
}
lldb::tid_t GetTID();
void SetTID(lldb::tid_t tid);

Expand All @@ -115,6 +117,9 @@ class ThreadPlanStack {
// completed plan checkpoints.
std::unordered_map<size_t, PlanStack> m_completed_plan_store;
mutable std::recursive_mutex m_stack_mutex;

// ThreadPlanStacks shouldn't be copied.
ThreadPlanStack(ThreadPlanStack &rhs) = delete;
};

class ThreadPlanStackMap {
Expand All @@ -128,15 +133,34 @@ class ThreadPlanStackMap {

void AddThread(Thread &thread) {
lldb::tid_t tid = thread.GetID();
m_plans_list.emplace(tid, thread);
// If we already have a ThreadPlanStack for this thread, use it.
if (m_plans_list.find(tid) != m_plans_list.end())
return;

m_plans_up_container.emplace_back(
std::make_unique<ThreadPlanStack>(thread));
m_plans_list.emplace(tid, m_plans_up_container.back().get());
}

bool RemoveTID(lldb::tid_t tid) {
auto result = m_plans_list.find(tid);
if (result == m_plans_list.end())
return false;
result->second.ThreadDestroyed(nullptr);
ThreadPlanStack *removed_stack = result->second;
m_plans_list.erase(result);
// Now find it in the stack storage:
auto end = m_plans_up_container.end();
auto iter = std::find_if(m_plans_up_container.begin(), end,
[&] (std::unique_ptr<ThreadPlanStack> &stack) {
return stack->IsTID(tid);
});
if (iter == end)
return false;

// Then tell the stack its thread has been destroyed:
removed_stack->ThreadDestroyed(nullptr);
// And then remove it from the container so it goes away.
m_plans_up_container.erase(iter);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: If haven't discovered it yet, there is also std::remove_if (https://en.cppreference.com/w/cpp/algorithm/remove). It works by moving the to-be-removed element to the end so you can std::erase it there (see the example).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll keep that in mind. ! I don't think that would make this code any clearer.

return true;
}

Expand All @@ -145,7 +169,7 @@ class ThreadPlanStackMap {
if (result == m_plans_list.end())
return nullptr;
else
return &result->second;
return result->second;
}

/// Clear the Thread* cache that each ThreadPlan contains.
Expand All @@ -154,38 +178,51 @@ class ThreadPlanStackMap {
/// generated.
void ClearThreadCache() {
for (auto &plan_list : m_plans_list)
plan_list.second.ClearThreadCache();
plan_list.second->ClearThreadCache();
}

// rename to Reactivate?
void Activate(ThreadPlanStack &&stack) {
void Activate(ThreadPlanStack &stack) {
// Remove this from the detached plan list:
auto end = m_detached_plans.end();
auto iter = std::find_if(m_detached_plans.begin(), end,
[&] (ThreadPlanStack *elem) {
return elem == &stack; });
if (iter != end)
m_detached_plans.erase(iter);

if (m_plans_list.find(stack.GetTID()) == m_plans_list.end())
m_plans_list.emplace(stack.GetTID(), std::move(stack));
m_plans_list.emplace(stack.GetTID(), &stack);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stack is a unique_ptr, right? Don't we want to call ::get here, or is the & overloaded for unique pointers?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"stack" is actually a ThreadPlanStack & passed into the function, not one we've pulled out of the container, where it would be a UP. So I think this is okay.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, and the address is held by the unique_ptr in the store, so it's stable which is why it's safe to take its address. That's why it caught my attention initially. Maybe it's worth adding a comment saying that (or changing the interface to take a pointer to be more explicit about it).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this to be a pointer, since I don't want to deal with getting passed a null, that's not allowed.

else
m_plans_list.at(stack.GetTID()) = std::move(stack);
m_plans_list.at(stack.GetTID()) = &stack;
}

// rename to ...?
std::vector<ThreadPlanStack> CleanUp() {
void ScanForDetachedPlanStacks() {
llvm::SmallVector<lldb::tid_t, 2> invalidated_tids;
for (auto &pair : m_plans_list)
if (pair.second.GetTID() != pair.first)
if (pair.second->GetTID() != pair.first)
invalidated_tids.push_back(pair.first);

std::vector<ThreadPlanStack> detached_stacks;
detached_stacks.reserve(invalidated_tids.size());
for (auto tid : invalidated_tids) {
auto it = m_plans_list.find(tid);
auto stack = std::move(it->second);
ThreadPlanStack *stack = it->second;
m_plans_list.erase(it);
detached_stacks.emplace_back(std::move(stack));
m_detached_plans.push_back(stack);
}
return detached_stacks;
}

// This gets the vector of pointers to thread plans that aren't
// currently running on a thread. This is generally for thread
// plans that represent asynchronous operations waiting to be
// scheduled.
// The vector will never have null ThreadPlanStacks in it.
std::vector<ThreadPlanStack *> &GetDetachedPlanStacks() {
return m_detached_plans;
}

void Clear() {
for (auto &plan : m_plans_list)
plan.second.ThreadDestroyed(nullptr);
plan.second->ThreadDestroyed(nullptr);
m_plans_list.clear();
}

Expand All @@ -202,7 +239,23 @@ class ThreadPlanStackMap {

private:
Process &m_process;
using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack>;
// We don't want to make copies of these ThreadPlanStacks, there needs to be
// just one of these tracking each piece of work. But we need to move the
// work from "attached to a TID" state to "detached" state, which is most
// conveniently done by having organizing containers for each of the two
// states.
// To make it easy to move these non-copyable entities in and out of the
// organizing containers, we make the ThreadPlanStacks into unique_ptr's in a
// storage container - m_plans_up_container. Storing unique_ptrs means we
// can then use the pointer to the ThreadPlanStack in the "organizing"
// containers, the TID->Stack map m_plans_list, and the detached plans
// vector m_detached_plans.

using PlansStore = std::vector<std::unique_ptr<ThreadPlanStack>>;
PlansStore m_plans_up_container;
std::vector<ThreadPlanStack *> m_detached_plans;

using PlansList = std::unordered_map<lldb::tid_t, ThreadPlanStack *>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I mentioned that in another review already, but if we store ThreadPlanStack * instead of ThreadPlanStack & shouldn't we check for nullptr at each use? In other words, could we store ThreadPlanStack & here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tried making these & not * but it seemed like that involved a lot more fighting with the compiler. This one is more straightforward. We will never have nullptr's in this list or return them so I'll add a comment to that effect.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment to the effect that the vector you get of ThreadPlanStack's will never have any nullptr entries. That is true by the way we deal with them.

PlansList m_plans_list;
};

Expand Down
18 changes: 8 additions & 10 deletions lldb/source/Target/Process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,25 +1286,23 @@ void Process::UpdateThreadListIfNeeded() {
}

void Process::SynchronizeThreadPlans() {
for (auto &stack : m_thread_plans.CleanUp())
m_async_thread_plans.emplace_back(std::move(stack));
m_thread_plans.ScanForDetachedPlanStacks();
}

ThreadPlanSP Process::FindDetachedPlanExplainingStop(Thread &thread,
Event *event_ptr) {
auto end = m_async_thread_plans.end();
for (auto it = m_async_thread_plans.begin(); it != end; ++it) {
auto plan_sp = it->GetCurrentPlan();
std::vector<ThreadPlanStack *> &detached_plans
= m_thread_plans.GetDetachedPlanStacks();
size_t num_detached_plans = detached_plans.size();
for (size_t idx = 0; idx < num_detached_plans; idx++) {
ThreadPlanStack *cur_stack = detached_plans[idx];
ThreadPlanSP plan_sp = cur_stack->GetCurrentPlan();
plan_sp->SetTID(thread.GetID());
if (!plan_sp->DoPlanExplainsStop(event_ptr)) {
plan_sp->ClearTID();
continue;
}

auto stack = std::move(*it);
m_async_thread_plans.erase(it);
stack.SetTID(plan_sp->GetTID());
m_thread_plans.Activate(std::move(stack));
m_thread_plans.Activate(*cur_stack);
return plan_sp;
}
return {};
Expand Down
6 changes: 3 additions & 3 deletions lldb/source/Target/ThreadPlanStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,8 @@ void ThreadPlanStackMap::DumpPlans(Stream &strm,
index_id = thread_sp->GetIndexID();

if (condense_if_trivial) {
if (!elem.second.AnyPlans() && !elem.second.AnyCompletedPlans() &&
!elem.second.AnyDiscardedPlans()) {
if (!elem.second->AnyPlans() && !elem.second->AnyCompletedPlans() &&
!elem.second->AnyDiscardedPlans()) {
strm.Printf("thread #%u: tid = 0x%4.4" PRIx64 "\n", index_id, tid);
strm.IndentMore();
strm.Indent();
Expand All @@ -466,7 +466,7 @@ void ThreadPlanStackMap::DumpPlans(Stream &strm,
strm.Indent();
strm.Printf("thread #%u: tid = 0x%4.4" PRIx64 ":\n", index_id, tid);

elem.second.DumpThreadPlans(strm, desc_level, internal);
elem.second->DumpThreadPlans(strm, desc_level, internal);
}
}

Expand Down