Skip to content
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

Jump to bottom

tls: maintain a free slot index set in TLS InstanceImpl to allocate in O(1… #7979

Merged
merged 4 commits into from
Aug 21, 2019
Merged

tls: maintain a free slot index set in TLS InstanceImpl to allocate in O(1… #7979

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12a2a95
maintain a free slot index set in TLS InstanceImpl to allocate in O(1…
stevenzzzz Aug 20, 2019
5708a82
change from hash set to list
stevenzzzz Aug 21, 2019
b636202
fix nit
stevenzzzz Aug 21, 2019
d1d43be
fix nit, one at a time
stevenzzzz Aug 21, 2019
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
20 changes: 11 additions & 9 deletions source/common/thread_local/thread_local_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ SlotPtr InstanceImpl::allocateSlot() {
ASSERT(std::this_thread::get_id() == main_thread_id_);
ASSERT(!shutdown_);

for (uint64_t i = 0; i < slots_.size(); i++) {
if (slots_[i] == nullptr) {
std::unique_ptr<SlotImpl> slot(new SlotImpl(*this, i));
slots_[i] = slot.get();
return slot;
}
if (free_slot_indexes_.empty()) {
std::unique_ptr<SlotImpl> slot(new SlotImpl(*this, slots_.size()));
slots_.push_back(slot.get());
return slot;
}

std::unique_ptr<SlotImpl> slot(new SlotImpl(*this, slots_.size()));
slots_.push_back(slot.get());
uint32_t idx = *free_slot_indexes_.begin();
free_slot_indexes_.erase(idx);
ASSERT(idx < slots_.size());
std::unique_ptr<SlotImpl> slot(new SlotImpl(*this, idx));
slots_[idx] = slot.get();
return slot;
}

Expand Down Expand Up @@ -73,6 +73,8 @@ void InstanceImpl::removeSlot(SlotImpl& slot) {

const uint64_t index = slot.index_;
slots_[index] = nullptr;
auto ret = free_slot_indexes_.emplace(index);
ASSERT(ret.second, fmt::format("slot index {} already in free slot set!", index));
Copy link
Member

Choose a reason for hiding this comment

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

With std::list you can do a similar assert using find

Copy link
Contributor Author

Choose a reason for hiding this comment

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

find is again O(N), and in this case it will go over the full list (as there won't be such a idx if we do things right here)
But if we don't care about the assertion, we can use std::list which is also O(1) allocation/return.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would just switch to list. I don't think the perf of the assertion matters. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change to std::list.
For some reason I thought ASSERT won't be compiled out. :/

runOnAllThreads([index]() -> void {
// This runs on each thread and clears the slot, making it available for a new allocations.
// This is safe even if a new allocation comes in, because everything happens with post() and
Expand Down
5 changes: 5 additions & 0 deletions source/common/thread_local/thread_local_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

#include "common/common/logger.h"

#include "absl/container/flat_hash_set.h"
Copy link
Member

Choose a reason for hiding this comment

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

nit: del

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


namespace Envoy {
namespace ThreadLocal {

Expand Down Expand Up @@ -57,6 +59,9 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>, public Instance {

static thread_local ThreadLocalData thread_local_data_;
std::vector<SlotImpl*> slots_;
// A set of index of freed slots.
absl::flat_hash_set<uint32_t> free_slot_indexes_;
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just use std::list here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we don't care about the assertion around when we return the slot, we can use a list here. Please let me know if you prefer to change to list.


std::list<std::reference_wrapper<Event::Dispatcher>> registered_threads_;
std::thread::id main_thread_id_;
Event::Dispatcher* main_thread_dispatcher_{};
Expand Down