[Tile-serving] [openstreetmap/osm2pgsql] Refactor pending processor (#1176)
Sarah Hoffmann
notifications at github.com
Wed May 20 21:03:42 UTC 2020
@lonvia commented on this pull request.
> @@ -272,6 +272,12 @@ struct pending_threaded_processor : public pending_processor
throw;
}
}
+
+ for (auto const &clone : m_clones) {
+ for (auto const &clone_output : clone) {
+ clone_output->commit();
+ }
+ }
Shouldn't that even be moved to the workers?
> for (auto const &clone : m_clones) {
- for (output_vec_t::const_iterator
- original_output = m_outputs.begin(),
- clone_output = clone.begin();
- original_output != m_outputs.end() &&
- clone_output != clone.end();
- ++original_output, ++clone_output) {
- //merge the expire tree from this threads copy of output back
- original_output->get()->merge_expire_trees(clone_output->get());
+ auto it = clone.begin();
+ for (auto const &output : m_outputs) {
+ assert(it != clone.end());
+ if (*it) {
+ output->merge_expire_trees(it->get());
+ }
+ ++it;
How about switching the for loops? Might make the code even more understandable.
>
static osmid_t pop_id(pending_queue_t &queue, std::mutex &mutex)
{
osmid_t id = 0;
std::lock_guard<std::mutex> const lock{mutex};
if (!queue.empty()) {
- id = queue.top();
- queue.pop();
+ id = queue.back();
+ queue.pop_back();
This reverts the order, right? Could that be an issue?
> @@ -43,18 +43,14 @@ bool full_dependency_manager_t::has_pending() const noexcept
return !m_ways_pending_tracker.empty() || !m_rels_pending_tracker.empty();
}
-void full_dependency_manager_t::process_pending(pending_processor &proc)
-{
- osmid_t id;
- while (id_tracker::is_valid(id = m_ways_pending_tracker.pop_mark())) {
- proc.enqueue_way(id);
- }
+idlist_t full_dependency_manager_t::get_ids(id_tracker& tracker) {
This commit is missing a clang-format run.
> }
private:
+ idlist_t get_ids(id_tracker& tracker);
can be static?
> + {
+ m_queue = std::move(list);
+ process_queue("relation", do_rels);
+
+ // Collect expiry tree information from all clones and merge it back
+ // into the original outputs.
+ for (auto const &clone : m_clones) {
+ auto it = clone.begin();
+ for (auto const &output : m_outputs) {
+ assert(it != clone.end());
+ if (*it) {
+ output->merge_expire_trees(it->get());
+ }
+ ++it;
+ }
+ }
Can we put this for loop in its own function? It has nothing to do with the relation processing, it is just another step to be done when all processing is done.
> @@ -297,40 +323,14 @@ struct pending_threaded_processor
}
}
- void process_ways(idlist_t &&list)
- {
- m_queue = std::move(list);
There really is no need anymore that m_queue is a member.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/osm2pgsql/pull/1176#pullrequestreview-415683669
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/tile-serving/attachments/20200520/d5225f01/attachment-0001.htm>
More information about the Tile-serving
mailing list