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.

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

[DNM][routing] Two threads bidirectional A*. #13929

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

bykoianko
Copy link
Contributor

@bykoianko bykoianko commented Nov 6, 2020

Решаемая задача:
На мобильных девайсах сейчас обычно 2 и более ядер. Как показали тесты, загрузка двух потоков обычно раскладывается шедулером на 2 ядра, даже на 2 ядерных девайсах (тесты были на iPhone SE). Мы прокладываем маршруты при помощи 2х стороннего A*. Прототип и тесты на нем (iPhone SE, Galaxy s10e) показали, что можно сделать, чтоб 2 потока работали при прокладке относительно не зависимо и, если каждому из них досталось свое ядро, то маршруты должны прокладываться существенно быстрее. Данный PR реализует это.

План работ:
Данный PR реализует двусторонний A* и структуры данных для него, которые позволяют искать маршруты, как в 2 потока (по одному на каждую волну), так и в один поток. Чтоб воспользоваться поиском маршрута в 2 потока нужно использовать SingleVehicleWorldGraph(IndexGraphStarter/IndexGraphStarterJoints) созданный для поиска в 2 потока.

После данного PR-та будет PR с тестами на эффективность и корректность, которые будут работать на приличной выборке маршрутов.
Затем будет PR с возможностью отключать двусторонний A* и использовать односторонний при помощи специального флажка, чтоб было проще тестировать в ручную.

В этот PR не войдет, хотя в будущем можно сделать:

  • поиск в 2 потока межmwm-го маршрута (шаг с leaps)
  • поиск в 2 потока при помощи: void SingleVehicleWorldGraph::GetEdgeList(astar::VertexData<**Segment**, RouteWeight> const & vertexData …). Данный PR реализацет поиск в 2 потока только при помощи: void SingleVehicleWorldGraph::GetEdgeList(astar::VertexData<**JointSegment**, RouteWeight> const & parentVertexData …)
  • поиск в 2 потока лучших сегментов около стартовой и конечной вершин.

Краткое описание алгоритма:
Основаная идея проста. 2 волны A* исполняются в 2 потоках раскрывая вершины на встречу друг другу. Пока они не соприкоснулись они движутся навстречу друг другу (это обычно основное время работы алгоритма) используется относительно мало общих данных. После соприкосновения волн работа алгоритма не заканчивается, но кол-во общих данных, используемых двумя волнами, существенно возрастает. Реализация в данном PR переключается на однопоточный вариант, как только волны соприкоснутся. Пока они не соприкоснулись есть только два набора общих данных: (1) очередь BidirectionalStepContext::bestDistance. Каждой волне важен доступ до очереди противоположенной волны, чтоб понять, пересеклись ли они; (2) Граф. Получение входящих и исходящих дуг, и вычисление эвристики.

Исследования показали, что если обращения к очереди (1) защитить мьютексом, то это не очень сильно сказывается на производительности.
Основные работы проведены над графом. Если сериализовать мьютексом получение входящих и исходящих дуг графа, то это критически повлияет на производительность. С другой стороны, этого и не нужно делать. В целом обращение за дугами, это операция чтения. Если сделать отдельные кеши (кеш полученной геометрии и кеш при файлом чтении) для обоих потоков, то останется синхронизировать очень не большие объемы кода (подргузка новых графов при распространении волны и т.п.). Так же, чтоб можно было получить входящие и исходящие дуги из разных потоков нужно сделать отдельные кеши для чтения с диска для каждого потока.

Корректность:
Математическая корректность - я постараюсь выделить время, на то чтоб доказать, что то что написано математически корректно. На интуитивном уровне кажется, что это так.
Тесты. Будут проведены и выложены в след за этим PR сравнительные тесты на большой выборке маршрутов.

Реализация
Идея в том, чтоб если используется поис маршрута в один поток (SingleVehicleWorldGraph(IndexGraphStarter/IndexGraphStarterJoints) созданный для поиска в 1 поток), то не должно быть существенных накладных расходов. Если же в 2 потока, то должны добавиться минимальные расходы на синхронизацию и дополнительные кеши для геометрии и кеш чтения с диска. В случае 2 потоков, используется второй поток для обратной волны A*, которая идет от финиша. Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing. Именно он определяет, для какой волны сейчас используются методы. В данном PR он прокинут в большое число методов.

Результаты сравнительных тестов на 16.11.2020
Первые тесты.
iPhone SE. Пеший маршрут: Тушино (Москва) - Таруса (55.857793, 37.409484; 54.719608, 37.194492).

8 прокладок подряд:
iPhone SE
Route build time: 44599 ms - 1 поток
Route build time: 31637 ms - 2 потока
Route build time: 30543 ms - 2 потока
Route build time: 39362 ms - 1 поток

Route build time: 40235 ms - 1 поток
Route build time: 30402 ms - 2 потока
Route build time: 30126 ms - 2 потока
Route build time: 39376 ms - 1 поток

s10e
Route build time: 14925 ms - 1 поток
Route build time: 9595 ms - 2 потока
Route build time: 9749 ms - 2 потока
Route build time: 14386 ms - 1 поток

Route build time: 14307 ms - 1 поток
Route build time: 9697 ms - 2 потока
Route build time: 9823 ms - 2 потока
Route build time: 14564 ms - 1 поток

Длина и ETA во всех прокладках одинаковая

Для сравнения две прокладки аналогичного маршрута на мастере (в один поток) на iPhoneSE дали:
Route build time: 44830 ms
Route build time: 40719 ms
Route build time: 39935 ms

routing integration tests и routing quality tests проходят.

@mesozoic-drones @mpimenov PTAL

@mesozoic-drones
Copy link
Contributor

mesozoic-drones commented Nov 6, 2020

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:

LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const

Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.

Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

@bykoianko
Copy link
Contributor Author

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:

LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const

Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.

Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

У нас isOutgoing именно и обозначает isWaveOutgoing. Но сейчас, по направлению волны мы ориентируемся еще и каким кэшом пользоваться при 2 поточной версии. isWaveOutgoing - определенно понятнее, но такое название удлинит очень много сигнатур:

grep -r -i --include \*.h --include \*.cpp isOutgoing ./ | wc -l
468

В свое время, мы хотели сделать не очень длинное имя для этого. Обсудим лучше голосом.

@bykoianko
Copy link
Contributor Author

bykoianko commented Nov 7, 2020

Вот так вернее:

grep -r -i --include \*.hpp --include \*.cpp isOutgoing ./ | wc -l
670

@gmoryes
Copy link
Contributor

gmoryes commented Nov 8, 2020

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:

LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const

Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.

Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

справедливости ради, isOutgoing - это не про волну, а про "исходящие"/"входящие" ребра

isWaveOutgoing - не очень понятно о чем, потому что и прямая и обратная волна "выходят" откуда-то

@bykoianko
Copy link
Contributor Author

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:
LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const
Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.
Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

справедливости ради, isOutgoing - это не про волну, а про "исходящие"/"входящие" ребра

isWaveOutgoing - не очень понятно о чем, потому что и прямая и обратная волна "выходят" откуда-то

Миша, привет! Да, это так. Т.е. у нас есть параметр, который изначально показывал какие ребра мы хотим поискать. Почти всегда мы ищем на волне от старта исходящие, а от финиша входящие ребра. А теперь еще и в 2х поточном варианте этот параметр указывает, какой кэш использовать. И в каком потоке вызывается метод. Может быть оставить isOutgoing/isIngoing и где-нибудь дать подробный комментарий, что мы имеем ввиду под этим параметром и в каких ситуациях?

@mesozoic-drones
Copy link
Contributor

Ок, давай как ты предложил с комментарием.

@gmoryes
Copy link
Contributor

gmoryes commented Nov 9, 2020

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:
LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const
Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.
Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

справедливости ради, isOutgoing - это не про волну, а про "исходящие"/"входящие" ребра
isWaveOutgoing - не очень понятно о чем, потому что и прямая и обратная волна "выходят" откуда-то

Миша, привет! Да, это так. Т.е. у нас есть параметр, который изначально показывал какие ребра мы хотим поискать. Почти всегда мы ищем на волне от старта исходящие, а от финиша входящие ребра. А теперь еще и в 2х поточном варианте этот параметр указывает, какой кэш использовать. И в каком потоке вызывается метод. Может быть оставить isOutgoing/isIngoing и где-нибудь дать подробный комментарий, что мы имеем ввиду под этим параметром и в каких ситуациях?

ну просто isOutgoung == true, говорит о том, что мы ищем исходящие ребра, что нужно для прямой волны и наоборот. То есть как бы да, isOutgoung - значит прямая волна, но косвенно. В терминах графа странно говорить "дай ребра, волна если что обратная", но в общем как удобнее вам)

@bykoianko
Copy link
Contributor Author

Какой из 2 кешей использовать, определяется при помощи параметра isOutgoing.

Название isOutgoing запутывает. Например, метод IndexGraphStarter:
LatLonWithAltitude const & GetJunction(Segment const & segment, bool front, bool isOutgoing) const
Здесь есть и bool front, и bool isOutgoing. Сбивает с толку, что к чему относится - к сегменту, кешу или чему-то еще.
Предлагаю переименовать на более длинное, но более понятное название. Как пример (можно предложить что-то лучше): bool isWaveOutgoing.

справедливости ради, isOutgoing - это не про волну, а про "исходящие"/"входящие" ребра
isWaveOutgoing - не очень понятно о чем, потому что и прямая и обратная волна "выходят" откуда-то

Миша, привет! Да, это так. Т.е. у нас есть параметр, который изначально показывал какие ребра мы хотим поискать. Почти всегда мы ищем на волне от старта исходящие, а от финиша входящие ребра. А теперь еще и в 2х поточном варианте этот параметр указывает, какой кэш использовать. И в каком потоке вызывается метод. Может быть оставить isOutgoing/isIngoing и где-нибудь дать подробный комментарий, что мы имеем ввиду под этим параметром и в каких ситуациях?

ну просто isOutgoung == true, говорит о том, что мы ищем исходящие ребра, что нужно для прямой волны и наоборот. То есть как бы да, isOutgoung - значит прямая волна, но косвенно. В терминах графа странно говорить "дай ребра, волна если что обратная", но в общем как удобнее вам)

Миша, спасибо за коммент! Я думаю, я напишу большой коммент на тему что isOutgoing означает и какие штуки сейчас к нему привязаны.

Comment on lines 277 to 288
, m_featureIdToRoad(
make_unique<RoutingFifoCache>(twoThreadsReady ? kRoadsCacheSizeTwoCaches : kRoadsCacheSize,
[this](uint32_t featureId, RoadGeometry & road) {
m_loader->Load(featureId, road, true /* isOutgoing */);
}))
, m_featureIdToRoadBwd(twoThreadsReady
? make_unique<RoutingFifoCache>(
kRoadsCacheSizeTwoCaches,
[this](uint32_t featureId, RoadGeometry & road) {
m_loader->Load(featureId, road, false /* isOutgoing */);
})
: nullptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

В инициализации полей m_featureIdToRoad, m_featureIdToRoadBwd участвует одна и та же лямбда. Может, вынести ее в отдельный метод?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поправил

@mesozoic-drones
Copy link
Contributor

mesozoic-drones commented Nov 10, 2020

for (auto twoThreadsReady : {false, true, true, false})

LOG TID(8) INFO      173.604 routing/index_router.cpp:419 CalculateRoute() --------------------- Two threads END---------------------
LOG TID(8) INFO      173.604 routing/index_router.cpp:413 CalculateRoute() --------------------- Two threads ---------------------

Тебе пока нужен этот лог? Можно оставить, главное перед мержем не забыть убрать.

Comment on lines 565 to 568
RouterResultCode IndexRouter::DoCalculateRoute(Checkpoints const & checkpoints,
m2::PointD const & startDirection,
RouterDelegate const & delegate, Route & route)
RouterDelegate const & delegate,
bool twoThreadsReady, Route & route)
Copy link
Contributor

Choose a reason for hiding this comment

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

Правильно ли я понимаю, что есть еще одна тудушка: в IndexRouter::CalculateRoute() прокидывать twoThreadsReady?

Потому что сейчас из IndexRouter::CalculateRoute() в цикле вызывается DoCalculateRoute() с twoThreadsReady=true и twoThreadsReady=false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Да. Цикл сейчас вызывается для лучшего тестирования. Я планирую оставить для прокладки маршрутов версию в 2 потока.

Comment on lines 37 to 38
IndexGraphLoaderImpl(VehicleType vehicleType, bool loadAltitudes, bool isTwoThreadsReady,
shared_ptr<NumMwmIds> numMwmIds,
Copy link
Contributor

Choose a reason for hiding this comment

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

Необязательно: унифицировать названия. У тебя в каких-то местах bool isTwoThreadsReady, а в каких-то - bool twoThreadsReady.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поправил в нескольких местах. IsTwoThreadsReady() это метод. А параметр twoThreadsReady.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Вернее даже так. Если мы намерены прокладывать в 2 потока то мы вызываем методы IndexRouter::CalculateRoute(), IndexRouter::DoCalculateRoute()и т.п. с параметром useTwoThreads. Но в IndexRouter::DoCalculateRoute() мы создаем WorldGraph. (unique_ptr<WorldGraph> graph = MakeWorldGraph(useTwoThreads);), который готов к тому, что его будут использовать в 2 потока, если useTwoThreads == true. С этого момента информация о том, во сколько потоков мы прокладываем хранится в графе и ни где не дублируется. Все просто. Если граф 2 поточный, то прокладываем в 2 потока. Если нет, то в один. С этого момента похожий параметр называется twoThreadsReady и соответствующие методы графов IsTwoThreadsReady(). Подробный комментарий на это счет есть около метода: FindPathBidirectional()

  /// \brief Finds path on |params.m_graph| using bidirectional A* algorithm.
  /// \note Two thread version is used when the version is set in |params.m_graph|.
  /// If |params.m_graph.IsTwoThreadsReady()| returns false, one thread version is used.
  /// It's worth using one thread version if there's only one core available.
  /// if |params.m_graph.IsTwoThreadsReady()| returns true two thread version is used.
  /// If the decision is made to use two thread version it should be taken into account:
  /// - |isOutgoing| flag in each method specified which thread calls the method
  /// - All callbacks which are called from |wave| lambda such as |params.m_onVisitedVertexCallback|
  ///   or |params.m_checkLengthCallback| should be ready for calling from two threads.

size_t constexpr kRoadsCacheSize = 5000;
// Maximum road geometry cache size in items in case twoThreadsReady == true.
size_t constexpr kRoadsCacheSizeTwoCaches = 3500;
Copy link
Contributor

Choose a reason for hiding this comment

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

Обсудили голосом: договорились, что если будет время, можно поварьировать параметр и замерить изменение скорости прокладки маршрута на девайсе.

Copy link
Contributor

@gmoryes gmoryes Nov 10, 2020

Choose a reason for hiding this comment

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

еще 3500 * 2 > 5000, это больше оперативки, если совсем не запариваться можно построить пешеходные маршруты на айфон 4 там, посмотреть, что приложение не падает на длинных маршрутах

но вообще можно профилирофщик на питоне запустить, это займет 20 минут примерно (там в конфиге нужно две ветки разные указать просто). Посмотреть - насколько больше памяти стали потреблять, я сходу не помню - сколько на кэш тратилось

Copy link
Contributor Author

@bykoianko bykoianko Nov 11, 2020

Choose a reason for hiding this comment

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

Обсудили голосом: договорились, что если будет время, можно поварьировать параметр и замерить изменение скорости прокладки маршрута на девайсе.

Да. Сделаю.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmoryes, даже более того. У нас при прокладке маршрута использовалось 3 кэша:

  1. Geometry::m_featureIdToRoad. 5000 вершин * sizeof(RoadGeometry) (его размер мы сейчас обсуждаем)
  2. AltitudeLoader::m_cache (размер не значительный, поскольку после использования сразу чистился)
  3. FeaturesRoadGraph::m_cache (используем из одного потока для поиска старта финиша и т.п.)

А для каждого из них, нужны ридеры, чтоб читать с диска, и у этих ридеров есть свои кеши для чтения с диска (FileReader).

Кеши (1) и (2) + соответствующие кеши чтения с диска удваиваются.

Я согласен. Надо провести спрофилировать по памяти на целевом устройстве. Я планирую использовать профилировщик xcode-да, но ты предлагал другой вариант.

но вообще можно профилирофщик на питоне запустить, это займет 20 минут примерно ...

Уточни п-та, что ты имел ввиду.

Copy link
Contributor

Choose a reason for hiding this comment

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

Уточни п-та, что ты имел ввиду.

ну я же писал специально )

вот питоновский скрипт:
https://github.com/mapsme/omim/blob/master/tools/python/routing/run_heap_comparison_benchmark.py

вот конфиг:
https://github.com/mapsme/omim/blob/master/tools/python/routing/etc/heap_comparison_benchmark.ini.example

в конфиге достаточно просто в OLD_VERSION указать одну ветку, а в NEW_VERSION указать другую ветку

и запустить на серваке, брать все 100к маршрутов тут не надо, потому что с профилировщиком памяти все работает очень долго, я бы взял там 5-10к

в --help написана инструкция, как назвать конфиг файлы

Copy link
Contributor

Choose a reason for hiding this comment

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

вообще в целом можно взять для начала 300 рандомных маршрутов, чтобы быстро все посчиталось, и потом если результаты будут обнадеживающие, то взять уже 5-10к

(чтобы не ждать там сутки, пока все посчитается и понять, что все очень плохо)

Comment on lines 80 to +82
FakeEnding MakeFakeEnding(Segment const & segment, m2::PointD const & point, IndexGraph & graph)
{
auto const & road = graph.GetGeometry().GetRoad(segment.GetFeatureId());
auto const & road = graph.GetGeometry().GetRoad(segment.GetFeatureId(), true /* isOutgoing */);
Copy link
Contributor

Choose a reason for hiding this comment

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

Обсудили голосом: в дальнейшем поиск FakeEnding для чекпоинтов можно также распараллелить. Можно оставить тудушку.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок. Напишу.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  /// @todo FindBestSegments() is called for start, finish and intermediate points of the route.
  /// On a modern mobile device this method takes 200-300ms. 
  /// The most number of routes have no intermediate points. It's worth calling this method
  /// for start on one thread and for finish and another one. It's not difficult to implement it
  /// based on functionality which was developed for two-thread bidirectional A*.

Comment on lines 222 to 223
/// If the decision is made to use two thread version it should be taken into account:
/// If the decision is made to use two thread version it should be taken into account:
Copy link
Contributor

Choose a reason for hiding this comment

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

Лишняя строчка

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Удалил.

@@ -181,6 +181,7 @@ optional<JointEdge> IndexGraph::GetJointEdgeByLastPoint(Segment const & parent,
Segment const & firstChild, bool isOutgoing,
uint32_t lastPoint)
{
// @TODO It should be ready for calling form two threads if IsTwoThreadsReady() returns true.
Copy link
Contributor

Choose a reason for hiding this comment

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

Опечатка form -> from

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Поправил. Впрочем, эти тудушки я планирую сделать до мержа.

@@ -557,8 +577,11 @@ AStarAlgorithm<Vertex, Edge, Weight>::FindPathBidirectional(P & params,
auto const & finalVertex = params.m_finalVertex;
auto const & startVertex = params.m_startVertex;

BidirectionalStepContext forward(true /* forward */, startVertex, finalVertex, graph);
BidirectionalStepContext backward(false /* forward */, startVertex, finalVertex, graph);
auto const useTwoThreads = graph.IsTwoThreadsReady();
Copy link
Contributor

Choose a reason for hiding this comment

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

Предлагаю перенести ближе к использованию.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я так высоко поднял, поскольку кажется, что это очень важная штука, по какому пути пойдет алгоритм. Чтоб сразу бросалась в глаза.

Copy link
Contributor

Choose a reason for hiding this comment

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

Тогда ок)

Comment on lines 603 to 605
// @TODO The multithreading code below in wave lambda is used more or less the same line
// as one thread bidirectional version. Consider put in some functions and call them for
// one thread and two thread versions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Да, было бы крайне неплохо, потому что вместе с твоим новым кодом этот метод уже длиннее 250 строчек)

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

@mesozoic-drones
Copy link
Contributor

mesozoic-drones commented Nov 10, 2020

@bykoianko Посмотрела реквест. Дай знать, как будет готов фикс undefined behavior в кросс-мвм.

@bykoianko
Copy link
Contributor Author

for (auto twoThreadsReady : {false, true, true, false})

LOG TID(8) INFO      173.604 routing/index_router.cpp:419 CalculateRoute() --------------------- Two threads END---------------------
LOG TID(8) INFO      173.604 routing/index_router.cpp:413 CalculateRoute() --------------------- Two threads ---------------------

Тебе пока нужен этот лог? Можно оставить, главное перед мержем не забыть убрать.

Да, пока желательно оставить. Я думаю, что перед тем, как сниму DNM я его удалю. Спасибо!

@bykoianko
Copy link
Contributor Author

Ок, давай как ты предложил с комментарием.

Добавил вот такой коммент в world_graph.hpp

/// \Note. About isOutgoing parameter.
/// In routing in hundreds of method isOutgoing boolean flag is used. This flag have
/// several meanings.
/// - Originally this parameter was added to distinguish getting ingoing graph edges and
///   outgoing graph edges. For example, if it's necessary to get outgoing edges
///   GetEdgeList(..., true /* isOutgoing */, ...) should be called and to get ingoing
///   graph edges GetEdgeList(..., false /* isOutgoing */, ...) should be called.
/// - On the other hand getting ingoing edges (isOutgoing == false) only for backward wave
///   in bidirectional A*. So it's possible to say that based on isOutgoing value
///   it's possible to say which wave in bidirectional A* is used.
/// - Then two-threads variant was implemented for bidirectional A*. A new thread is created
///   for backward A* bidirectional wave in this case. So if isOutgoing == false
///   that means the method is called from this additional thread.

@bykoianko
Copy link
Contributor Author

@bykoianko Посмотрела реквест. Дай знать, как будет готов фикс undefined behavior в кросс-мвм.

Спасибо! Это следующее дело. Пока доделал получение высоты из 2 потоков и поправил по комментам.

@mesozoic-drones @mpimenov PTAL

@bykoianko
Copy link
Contributor Author

@mesozoic-drones Добавил синхронизацию при обращении к кросс-mwm-ым секциям.

Comment on lines 607 to 608
// @TODO |startSegments| and |finishSegments| may be found in two threads with the help of
// |graph| with two threads support.
Copy link
Contributor

Choose a reason for hiding this comment

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

Какая-то расплывчатая тудушка. Можешь объяснить, что именно ты здесь планируешь сделать?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я ее удалил, потому, что вместо нее есть четкая:

  /// @todo FindBestSegments() is called for start, finish and intermediate points of the route.
  /// On a modern mobile device this method takes 200-300ms.
  /// The most number of routes have no intermediate points. It's worth calling this method
  /// for start on one thread and for finish and another one. It's not difficult to implement it
  /// based on functionality which was developed for two-thread bidirectional A*.
  bool FindBestSegments(m2::PointD const & checkpoint, m2::PointD const & direction, bool isOutgoing,	  bool FindBestSegments(m2::PointD const & checkpoint, m2::PointD const & direction, bool isOutgoing,
                        WorldGraph & worldGraph, std::vector<Segment> & bestSegments);	                        WorldGraph & worldGraph, std::vector<Segment> & bestSegments);

ASSERT(!queue.empty(), ());
return bestDistance.at(queue.top().vertex);
ASSERT(stateV || !queue.empty(),
("Either stateV should have value or queue should have value(s)."));
Copy link
Contributor

Choose a reason for hiding this comment

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

На твое усмотрение: тут можно без комментария в ассерте, потому что он фактически дублирует сам ассерт и никакой дополнительной информации не привносит.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я бы оставил коммент... Если ASSERT сработает, то кроме номера строчки мы увидим какой-то текст, на который можно будет сориентироваться, до просмотра кода. Или представь, что ASSERT будет в старой версии и строчка уже уедет. Тут тест тоже придаст уверенности, как мне кажется...

@@ -386,10 +387,10 @@ class AStarAlgorithm
graph.GetIngoingEdgesList(data, adj);
}

State GetState(bool takesCachedState)
State TopAndPopState()
Copy link
Contributor

Choose a reason for hiding this comment

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

Тебе не кажется, что новое название только наполовину отражает суть происходящего?
В случае, если опшнл stateV заполнен, мы берем его значение и сбрасываем сам опшнл. Из названия кажется, что в любом случае ожидается top+pop очереди. Предлагаю оставить старое GetState().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Да. Ты права. Но если посмотреть на использование метода GetState(), то это будет выглядеть во так:

    auto const firstStepAfterTwoThreads = cur->stateV != std::nullopt;

    State const stateV = cur->GetState();

    if (cur->ExistsStateWithBetterDistance(stateV))
      continue;
    if (!firstStepAfterTwoThreads)
    {
...

Ничего не хочется поправить при таком коде? ))) Мне бы захотелось убрать переменную firstStepAfterTwoThreads или объявить ее перед использованием. Если оставить GetState(), то у нас будет getter меняющий состояние. Как вариант: TopAndPopStateIfNoCachedState()?

Copy link
Contributor

Choose a reason for hiding this comment

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

Можно GetState() переименовать на UpdateState(), потому что он изменяет очередь либо изменяет stateV. Еще можно оставить комментарий, чтобы ни у кого не возникало идеи заниматься описанной тобой оптимизацией с firstStepAfterTwoThreads)

@mpimenov Ты бы как сделал?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Еще можно оставить комментарий, чтобы ни у кого не возникало идеи заниматься описанной тобой оптимизацией с firstStepAfterTwoThreads)

Он уже оставлен на всякий случай :). UpdateState() как вариант, можно.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Я бы это назвал PollState(), заимствуя у java.util.Queue.
  2. Кэширование значения (stateV) я бы не выносил в название метода, это деталь реализации.
  3. В текущей реализации я бы избавился от тернарного оператора: так как после него всё равно ветвление по тому же условию, нужно оставить один if и в его ветках всё сделать, будет читаемее.
State state;
if (stateV)
{
  state = *stateV;
  stateV = std::nullopt;
}
else
{
  state = queue.top();
  queue.pop();
}
ASSERT(!stateV, ());
return state;
  1. Я бы поисследовал возможность не добавлять кэширование stateV, т.е. не усложнять это место.

…two-threads bidirectional a star instead of two flags.
@bykoianko bykoianko changed the title [DNM][routing] Two threads bidirectional A*. [routing] Two threads bidirectional A*. Nov 19, 2020
@bykoianko bykoianko changed the title [routing] Two threads bidirectional A*. [DNM][routing] Two threads bidirectional A*. Nov 30, 2020
Copy link
Contributor

@mpimenov mpimenov left a comment

Choose a reason for hiding this comment

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

Оказалось, у меня здесь было несколько неотправленных комментариев.

std::unique_ptr<CopiedMemoryRegion> m_altitudeAvailabilityRegion;
std::unique_ptr<CopiedMemoryRegion> m_featureTableRegion;

succinct::rs_bit_vector m_altitudeAvailability;
succinct::elias_fano m_featureTable;

// Note. If |twoThreadsReady| parameter of constructor is true method GetAltitudes() may be called
// from two different threads. In that case all calls of GetAltitudes() with |isOutgoing| == true
// should be done from one thread and from another one of |isOutgoing| == true.
Copy link
Contributor

Choose a reason for hiding this comment

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

// In that case all calls of GetAltitudes() with different values
// of |isOutgoing| (true or false) must be made from different threads
// and never mixed up.

@@ -108,14 +108,14 @@ class GeometryLoader
public:
virtual ~GeometryLoader() = default;

virtual void Load(uint32_t featureId, RoadGeometry & road) = 0;
virtual void Load(uint32_t featureId, RoadGeometry & road, bool isOutgoing) = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Выходной параметр последним?

@@ -319,6 +319,9 @@ void IndexGraph::GetSegmentCandidateForJoint(Segment const & parent, bool isOutg
/// \param |parentWeights| - see |IndexGraphStarterJoints::GetEdgeList| method about this argument.
/// Shortly - in case of |isOutgoing| == false, method saves here the weights
/// from parent to firstChildren.
/// \note Despite the fact the method is not constant it still may be called from two
/// threads. One should call it with |isOutgoing| == true and another one with
Copy link
Contributor

Choose a reason for hiding this comment

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

s/One/One thread/
s/another/the other/

@@ -95,7 +95,10 @@ class IndexGraphStarter : public AStarGraph<IndexGraph::Vertex, IndexGraph::Edge

// Checks whether |weight| meets non-pass-through crossing restrictions according to placement of
// start and finish in pass-through/non-pass-through area and number of non-pass-through crosses.
bool CheckLength(RouteWeight const & weight);
// Note. CheckLength() may be called from two different threads according to |isOutgoing|
// but it no need additional synchronization. Although |m_start| and |m_finish| may be
Copy link
Contributor

Choose a reason for hiding this comment

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

s/no need/needs no/

size_t constexpr kRoadsCacheSize = 5000;
// Maximum road geometry cache size in items in case twoThreadsReady == true.
Copy link
Contributor

Choose a reason for hiding this comment

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

это размер общего для двух тредов кэша или размеры кэшей на каждом из потоков?

@@ -77,6 +77,11 @@ class IndexRouter : public IRouter
traffic::TrafficCache const & trafficCache, DataSource & dataSource);

std::unique_ptr<WorldGraph> MakeSingleMwmWorldGraph();
/// @todo FindBestSegments() is called for start, finish and intermediate points of the route.
/// On a modern mobile device this method takes 200-300ms.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/modern/modern (2020)/

@@ -77,6 +77,11 @@ class IndexRouter : public IRouter
traffic::TrafficCache const & trafficCache, DataSource & dataSource);

std::unique_ptr<WorldGraph> MakeSingleMwmWorldGraph();
/// @todo FindBestSegments() is called for start, finish and intermediate points of the route.
/// On a modern mobile device this method takes 200-300ms.
/// The most number of routes have no intermediate points. It's worth calling this method
Copy link
Contributor

Choose a reason for hiding this comment

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

s/The most number of routes/Most routes/

/// @todo FindBestSegments() is called for start, finish and intermediate points of the route.
/// On a modern mobile device this method takes 200-300ms.
/// The most number of routes have no intermediate points. It's worth calling this method
/// for start on one thread and for finish and another one. It's not difficult to implement it
Copy link
Contributor

Choose a reason for hiding this comment

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

s/finish and another one/finish on another one/
?

++counter;
if (counter % m_visitPeriod == 0)
return true;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

return counter % m_visitPeriod == 0;
?

@@ -386,10 +387,10 @@ class AStarAlgorithm
graph.GetIngoingEdgesList(data, adj);
}

State GetState(bool takesCachedState)
State TopAndPopState()
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Я бы это назвал PollState(), заимствуя у java.util.Queue.
  2. Кэширование значения (stateV) я бы не выносил в название метода, это деталь реализации.
  3. В текущей реализации я бы избавился от тернарного оператора: так как после него всё равно ветвление по тому же условию, нужно оставить один if и в его ветках всё сделать, будет читаемее.
State state;
if (stateV)
{
  state = *stateV;
  stateV = std::nullopt;
}
else
{
  state = queue.top();
  queue.pop();
}
ASSERT(!stateV, ());
return state;
  1. Я бы поисследовал возможность не добавлять кэширование stateV, т.е. не усложнять это место.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants