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

feat(search): Support mutex to guarantee index update thread safety #2491

Closed
wants to merge 1 commit into from
Closed

feat(search): Support mutex to guarantee index update thread safety #2491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion src/search/index_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <algorithm>
#include <map>
#include <memory>
#include <mutex>
#include <string>
#include <utility>

Expand Down Expand Up @@ -52,21 +53,28 @@ struct FieldInfo {

struct IndexInfo {
using FieldMap = std::map<std::string, FieldInfo>;
using MutexMap = std::map<std::string, std::mutex>;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can put mutexes into IndexUpdaters instead of IndexInfos?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this makes more sense <3

Copy link
Member Author

Choose a reason for hiding this comment

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

While trying to move the exact same piece of code into IndexUpdaters, I encountered the following errors:

‘constexpr std::pair<_T1, _T2>::pair(const std::pair<_T1, _T2>&) [with _T1 = const std::__cxx11::basic_string<char>; _T2 = std::mutex]’ is implicitly deleted because the default definition would be ill-formed:
  314 |       constexpr pair(const pair&) = default;    ///< Copy constructor

I suspect that this is caused by IndexUpdaters are usually created on stacks, which causes std::map doing some underlying copies and deletions, while IndexInfos are allocated on heap so doesn't have the issue.
I tried to wrap std::mutex into a unique_ptr but the compiler will report the same error.

I'm not quite sure what I can do at this point, may leave it as it is, and try to benchmark first. But feel free to let me know if you think there is anything I missed, or any workaround you can think of. @PragmaTwice <3

Copy link
Member

@PragmaTwice PragmaTwice Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for the delayed response.

I checked the related code, and I think there's two methods to solve it:

  1. keep mutex maps inside GlobalIndexer, and add a raw pointer into IndexUpdater to access them;
  2. follow the current way and modify methods like void Add(IndexUpdater updater) to Add(std::unqiue_ptr<IndexUpdater>).

Copy link
Member Author

Choose a reason for hiding this comment

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

All good all good, thanks for sharing your solution!
Might be a bit busy these two weeks, will be back to work on it during mid October.


std::string name;
redis::IndexMetadata metadata;
FieldMap fields;
mutable MutexMap field_mutexes;
redis::IndexPrefixes prefixes;
std::string ns;

IndexInfo(std::string name, redis::IndexMetadata metadata, std::string ns)
: name(std::move(name)), metadata(std::move(metadata)), ns(std::move(ns)) {}

void Add(FieldInfo &&field) {
const auto &name = field.name;
auto name = field.name;
field.index = this;
fields.emplace(name, std::move(field));
field_mutexes.emplace(std::piecewise_construct, std::make_tuple(name), std::make_tuple());
}

void LockField(const std::string &field_name) const { field_mutexes.at(field_name).lock(); }

void UnLockField(const std::string &field_name) const { field_mutexes.at(field_name).unlock(); }
};

struct IndexMap : std::map<std::string, std::unique_ptr<IndexInfo>> {
Expand Down
14 changes: 9 additions & 5 deletions src/search/indexer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,19 +304,23 @@ Status IndexUpdater::UpdateIndex(const std::string &field, std::string_view key,
return {Status::NotOK, "No such field to do index updating"};
}

info->LockField(field);

auto *metadata = iter->second.metadata.get();
SearchKey search_key(info->ns, info->name, field);
Status status;
if (auto tag = dynamic_cast<TagFieldMetadata *>(metadata)) {
GET_OR_RET(UpdateTagIndex(key, original, current, search_key, tag));
status = UpdateTagIndex(key, original, current, search_key, tag);
} else if (auto numeric [[maybe_unused]] = dynamic_cast<NumericFieldMetadata *>(metadata)) {
GET_OR_RET(UpdateNumericIndex(key, original, current, search_key, numeric));
status = UpdateNumericIndex(key, original, current, search_key, numeric);
} else if (auto vector = dynamic_cast<HnswVectorFieldMetadata *>(metadata)) {
GET_OR_RET(UpdateHnswVectorIndex(key, original, current, search_key, vector));
status = UpdateHnswVectorIndex(key, original, current, search_key, vector);
} else {
return {Status::NotOK, "Unexpected field type"};
status = {Status::NotOK, "Unexpected field type"};
}

return Status::OK();
info->UnLockField(field);
return status;
}

Status IndexUpdater::Update(const FieldValues &original, std::string_view key) const {
Expand Down