Concurrent graph modifications#321
Conversation
razdoburdin
requested review from
ahuber21,
ethanglaser and
ibhati
as code owners
| using index_type = Idx; | ||
| using value_type = std::span<Idx>; | ||
| using const_value_type = std::span<const Idx>; | ||
| using const_value_type = AtomicSpan<const Idx>; |
There was a problem hiding this comment.
For better flexibility and allow user to select syncronized/non-syncronized index kind, I would define a dedicated SyncronizedGraphBase class.
There was a problem hiding this comment.
I am not sure this duplication is really required. Performance penalty for synchronized vs non-synchronized search is only few precents.
There was a problem hiding this comment.
Hi @razdoburdin, is duplication too bad in this case? Otherwise, @rfsaliev may have a point on flexibility and it's always better if performance is not affected. But agree with you that pros and cons should be discussed if duplication is a large overhead.
There was a problem hiding this comment.
I plan to investigate the trade-offs, after the finalization of design of concurrent path. Let's make sure it works well first.
There was a problem hiding this comment.
To get valuable results, I would recommend to benchmark 'static' VamanaIndex with and without synchronized graph on different platforms - especially on multi-socket systems.
| if (is_deleted(dst)) { | ||
| const auto& others = graph_.get_node(dst); | ||
| all_candidates.insert(others.begin(), others.end()); | ||
| // SeqLock retry: a concurrent consolidate may be writing dst's |
There was a problem hiding this comment.
GraphConsolidator class is parametrized by a graph type, so we can keep both syncronized/non-synchronized behavior by detecting graph type.
3 participants
This PR introduces concurrent graph modifications with seqlock pattern.