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

[Beihao-Zhou]

Closes #2489

Summary

This PR fixes the concurrency issue mentioned in #2481, however, this is now pretty slow for uploading operation and bounded by the computational resources from my local laptop. I'll set up the same instance capacity as mentioned in the post and benchmark.

[PragmaTwice]

Maybe we can put mutexes into IndexUpdaters instead of IndexInfos?

[Beihao-Zhou]

Sure, this makes more sense <3

[Beihao-Zhou]

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

[PragmaTwice]

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>).

[Beihao-Zhou]

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.

[Beihao-Zhou]

Tried the benchmark framework today, which took more than one hour to upload, but didn't even finish. I thought there was something wrong and killed the process, but after double-checking the redis benchmark here, seems like it also took redis 1.5 hours.
Will try it again these two days, but one call-out is that kvrocks definitely doesn't fully utilize the compute resources.

[jonathanc-n]

@Beihao-Zhou Do you know if theres a place where you can configure your compute for kvrocks?

[Beihao-Zhou]

@Beihao-Zhou Do you know if theres a place where you can configure your compute for kvrocks?

Thanks a lot for pointing this out! I thought I was bottlenecked by lock itself so didn't try different config options, will look into them!

[Beihao-Zhou]

Apology for the late update! Was busy with relocation for the past two weeks.

Quick Update:
I tried to benchmark the default dataset upload operation, but it took around 4.5 hours and didn't even finish...
I can't quite estimate how long it will take, but notice that only one thread will use 100% CPU during the majority time. I'm not quite sure if this is because we used mutex here.
I'll try to tune CPU usage in the config and run the benchmark again these two days; however, let me know if you have any better ideas! @PragmaTwice

[PragmaTwice]

I'll take this task and submit a patch for it : )

[Beihao-Zhou]

I'll take this task and submit a patch for it : )

Thanks a ton!! Was too distracted by work these days.... My apology 😭