From 3e5daee1066aa67f1199c6f414800d8e786b3739 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Sat, 19 Aug 2023 14:33:16 +0200 Subject: [PATCH 1/8] Move idlist_t into its own header file --- src/dependency-manager.hpp | 1 + src/idlist.hpp | 34 ++++++++++++++++++++++++++++++++++ src/middle-pgsql.cpp | 1 + src/osmdata.hpp | 1 + src/osmtypes.hpp | 17 ----------------- src/pgsql-helper.hpp | 1 + tests/common-buffer.hpp | 1 + 7 files changed, 39 insertions(+), 17 deletions(-) create mode 100644 src/idlist.hpp diff --git a/src/dependency-manager.hpp b/src/dependency-manager.hpp index 3afcc88c8..18358c8e5 100644 --- a/src/dependency-manager.hpp +++ b/src/dependency-manager.hpp @@ -10,6 +10,7 @@ * For a full list of authors see the git log. */ +#include "idlist.hpp" #include "osmtypes.hpp" #include diff --git a/src/idlist.hpp b/src/idlist.hpp new file mode 100644 index 000000000..bd75be395 --- /dev/null +++ b/src/idlist.hpp @@ -0,0 +1,34 @@ +#ifndef OSM2PGSQL_IDLIST_HPP +#define OSM2PGSQL_IDLIST_HPP + +/** + * SPDX-License-Identifier: GPL-2.0-or-later + * + * This file is part of osm2pgsql (https://osm2pgsql.org/). + * + * Copyright (C) 2006-2023 by the osm2pgsql developer community. + * For a full list of authors see the git log. + */ + +/** + * \file + * + * This file contains the definition of the idlist_t class. + */ + +#include "osmtypes.hpp" + +#include + +struct idlist_t : public std::vector +{ + // Get all constructors from std::vector + using vector::vector; + + // Even though we got all constructors from std::vector we need this on + // some compilers/libraries for some reason. + idlist_t() = default; + +}; + +#endif // OSM2PGSQL_IDLIST_HPP diff --git a/src/middle-pgsql.cpp b/src/middle-pgsql.cpp index 824eba9a9..9b169ffdc 100644 --- a/src/middle-pgsql.cpp +++ b/src/middle-pgsql.cpp @@ -33,6 +33,7 @@ #include #include "format.hpp" +#include "idlist.hpp" #include "json-writer.hpp" #include "logging.hpp" #include "middle-pgsql.hpp" diff --git a/src/osmdata.hpp b/src/osmdata.hpp index 78b6ddee1..f632cd4ee 100644 --- a/src/osmdata.hpp +++ b/src/osmdata.hpp @@ -24,6 +24,7 @@ #include #include "dependency-manager.hpp" +#include "idlist.hpp" #include "osmtypes.hpp" #include "pgsql-params.hpp" diff --git a/src/osmtypes.hpp b/src/osmtypes.hpp index 6eadff83b..b8b2dee66 100644 --- a/src/osmtypes.hpp +++ b/src/osmtypes.hpp @@ -238,23 +238,6 @@ class taglist_t std::vector m_tags; }; // class taglist_t -struct idlist_t : public std::vector -{ - // Get all constructors from std::vector - using vector::vector; - - // Even though we got all constructors from std::vector we need this on - // some compilers/libraries for some reason. - idlist_t() = default; - - explicit idlist_t(osmium::NodeRefList const &list) - { - for (auto const &n : list) { - push_back(n.ref()); - } - } -}; - using rolelist_t = std::vector; #endif // OSM2PGSQL_OSMTYPES_HPP diff --git a/src/pgsql-helper.hpp b/src/pgsql-helper.hpp index 15a46785d..7a38ee753 100644 --- a/src/pgsql-helper.hpp +++ b/src/pgsql-helper.hpp @@ -10,6 +10,7 @@ * For a full list of authors see the git log. */ +#include "idlist.hpp" #include "osmtypes.hpp" #include diff --git a/tests/common-buffer.hpp b/tests/common-buffer.hpp index 251d6c553..a974ef36d 100644 --- a/tests/common-buffer.hpp +++ b/tests/common-buffer.hpp @@ -11,6 +11,7 @@ */ #include "format.hpp" +#include "idlist.hpp" #include "osmtypes.hpp" #include From 65a9317baabc626fefc41d2987979e057b1cad24 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Sat, 19 Aug 2023 15:03:45 +0200 Subject: [PATCH 2/8] Make osmid_t a real class --- src/idlist.hpp | 50 +++++++++++++++++++++++++++++++++++++------ src/osmdata.cpp | 3 +-- tests/test-middle.cpp | 10 ++++----- 3 files changed, 50 insertions(+), 13 deletions(-) diff --git a/src/idlist.hpp b/src/idlist.hpp index bd75be395..44a785564 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -18,17 +18,55 @@ #include "osmtypes.hpp" +#include #include -struct idlist_t : public std::vector +class idlist_t { - // Get all constructors from std::vector - using vector::vector; +public: + using value_type = osmid_t; - // Even though we got all constructors from std::vector we need this on - // some compilers/libraries for some reason. idlist_t() = default; -}; + idlist_t(std::initializer_list ids) : m_list(ids) {} + + bool empty() const noexcept { return m_list.empty(); } + + std::size_t size() const noexcept { return m_list.size(); } + + auto begin() const noexcept { return m_list.begin(); } + + auto end() const noexcept { return m_list.end(); } + + osmid_t operator[](std::size_t n) const noexcept { return m_list[n]; } + + void clear() { m_list.clear(); } + + void push_back(osmid_t id) { m_list.push_back(id); } + + void reserve(std::size_t size) { m_list.reserve(size); } + + osmid_t pop_id() + { + assert(!m_list.empty()); + auto const id = m_list.back(); + m_list.pop_back(); + return id; + } + + friend bool operator==(idlist_t const &lhs, idlist_t const &rhs) noexcept + { + return lhs.m_list == rhs.m_list; + } + + friend bool operator!=(idlist_t const &lhs, idlist_t const &rhs) noexcept + { + return !(lhs == rhs); + } + +private: + std::vector m_list; + +}; // class idlist_t #endif // OSM2PGSQL_IDLIST_HPP diff --git a/src/osmdata.cpp b/src/osmdata.cpp index f2a20af2c..6051194b3 100644 --- a/src/osmdata.cpp +++ b/src/osmdata.cpp @@ -244,8 +244,7 @@ class multithreaded_processor std::lock_guard const lock{*mutex}; if (!queue->empty()) { - id = queue->back(); - queue->pop_back(); + id = queue->pop_id(); } return id; diff --git a/tests/test-middle.cpp b/tests/test-middle.cpp index 9f36536fb..f384672bd 100644 --- a/tests/test-middle.cpp +++ b/tests/test-middle.cpp @@ -1118,7 +1118,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, REQUIRE(dependency_manager.has_pending()); idlist_t const way_ids = dependency_manager.get_pending_way_ids(); - REQUIRE_THAT(way_ids, Catch::Equals({20})); + REQUIRE(way_ids == idlist_t{20}); check_way(mid, way20); check_way_nodes(mid, way20.id(), {&node10a, &node11}); @@ -1153,7 +1153,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in way", "", options_slim_default, REQUIRE(dependency_manager.has_pending()); idlist_t const way_ids = dependency_manager.get_pending_way_ids(); - REQUIRE_THAT(way_ids, Catch::Equals({20, 22})); + REQUIRE(way_ids == idlist_t{20, 22}); check_way(mid, way20); check_way_nodes(mid, way20.id(), {&node10a, &node11}); @@ -1265,7 +1265,7 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default, REQUIRE(dependency_manager.has_pending()); idlist_t const rel_ids = dependency_manager.get_pending_relation_ids(); - REQUIRE_THAT(rel_ids, Catch::Equals({30})); + REQUIRE(rel_ids == idlist_t{30}); check_relation(mid, rel30); } @@ -1286,9 +1286,9 @@ TEMPLATE_TEST_CASE("middle: change nodes in relation", "", options_slim_default, REQUIRE(dependency_manager.has_pending()); idlist_t const way_ids = dependency_manager.get_pending_way_ids(); - REQUIRE_THAT(way_ids, Catch::Equals({20})); + REQUIRE(way_ids == idlist_t{20}); idlist_t const rel_ids = dependency_manager.get_pending_relation_ids(); - REQUIRE_THAT(rel_ids, Catch::Equals({31})); + REQUIRE(rel_ids == idlist_t{31}); check_relation(mid, rel31); } } From ba8ef220ab27b2f0ce4ffd85656ac3e0099301f7 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Sat, 19 Aug 2023 15:28:05 +0200 Subject: [PATCH 3/8] Refactor: Use idlist_t instead of osmium::index::IdSetSmall They are both basically the same class. We need a few functions from IdSetSmall though. --- src/CMakeLists.txt | 1 + src/dependency-manager.cpp | 19 ++++++++----------- src/dependency-manager.hpp | 20 ++++++++------------ src/idlist.cpp | 33 +++++++++++++++++++++++++++++++++ src/idlist.hpp | 13 +++++++++++++ src/middle-pgsql.cpp | 20 ++++++++------------ src/middle-pgsql.hpp | 12 +++++------- src/middle.hpp | 14 ++++++-------- src/output-flex.cpp | 4 ++-- src/output-flex.hpp | 8 +++----- src/output.hpp | 8 ++++---- 11 files changed, 91 insertions(+), 61 deletions(-) create mode 100644 src/idlist.cpp diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 528ff6be1..8ddf41219 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -19,6 +19,7 @@ target_sources(osm2pgsql_lib PRIVATE geom-from-osm.cpp geom-functions.cpp geom-pole-of-inaccessibility.cpp + idlist.cpp input.cpp logging.cpp middle.cpp diff --git a/src/dependency-manager.cpp b/src/dependency-manager.cpp index 72f1fe42e..534d42bfc 100644 --- a/src/dependency-manager.cpp +++ b/src/dependency-manager.cpp @@ -15,17 +15,17 @@ void full_dependency_manager_t::node_changed(osmid_t id) { - m_changed_nodes.set(id); + m_changed_nodes.push_back(id); } void full_dependency_manager_t::way_changed(osmid_t id) { - m_changed_ways.set(id); + m_changed_ways.push_back(id); } void full_dependency_manager_t::relation_changed(osmid_t id) { - m_changed_relations.set(id); + m_changed_relations.push_back(id); } void full_dependency_manager_t::after_nodes() @@ -39,15 +39,13 @@ void full_dependency_manager_t::after_nodes() m_changed_nodes.clear(); } -static osmium::index::IdSetSmall -set_diff(osmium::index::IdSetSmall const &set, - osmium::index::IdSetSmall const &to_be_removed) +static idlist_t set_diff(idlist_t const &set, idlist_t const &to_be_removed) { - osmium::index::IdSetSmall new_set; + idlist_t new_set; for (auto const id : set) { if (!to_be_removed.get_binary_search(id)) { - new_set.set(id); + new_set.push_back(id); } } @@ -92,7 +90,7 @@ void full_dependency_manager_t::after_relations() } void full_dependency_manager_t::mark_parent_relations_as_pending( - osmium::index::IdSetSmall const &way_ids) + idlist_t const &way_ids) { assert(m_rels_pending_tracker.empty()); m_object_store->get_way_parents(way_ids, &m_rels_pending_tracker); @@ -103,8 +101,7 @@ bool full_dependency_manager_t::has_pending() const noexcept return !m_ways_pending_tracker.empty() || !m_rels_pending_tracker.empty(); } -idlist_t -full_dependency_manager_t::get_ids(osmium::index::IdSetSmall *tracker) +idlist_t full_dependency_manager_t::get_ids(idlist_t *tracker) { tracker->sort_unique(); diff --git a/src/dependency-manager.hpp b/src/dependency-manager.hpp index 18358c8e5..e5271e89e 100644 --- a/src/dependency-manager.hpp +++ b/src/dependency-manager.hpp @@ -13,8 +13,6 @@ #include "idlist.hpp" #include "osmtypes.hpp" -#include - #include #include #include @@ -59,8 +57,7 @@ class dependency_manager_t virtual void after_ways() {} virtual void after_relations() {} - virtual void mark_parent_relations_as_pending( - osmium::index::IdSetSmall const & /*way_ids*/) + virtual void mark_parent_relations_as_pending(idlist_t const & /*way_ids*/) { } @@ -112,8 +109,7 @@ class full_dependency_manager_t : public dependency_manager_t void after_ways() override; void after_relations() override; - void mark_parent_relations_as_pending( - osmium::index::IdSetSmall const &ids) override; + void mark_parent_relations_as_pending(idlist_t const &ids) override; bool has_pending() const noexcept override; @@ -128,7 +124,7 @@ class full_dependency_manager_t : public dependency_manager_t } private: - static idlist_t get_ids(osmium::index::IdSetSmall *tracker); + static idlist_t get_ids(idlist_t *tracker); std::shared_ptr m_object_store; @@ -140,7 +136,7 @@ class full_dependency_manager_t : public dependency_manager_t * the change file, too, and so we don't have to find out which ones they * are. */ - osmium::index::IdSetSmall m_changed_nodes; + idlist_t m_changed_nodes; /** * In append mode all new and changed ways will be added to this. After @@ -149,17 +145,17 @@ class full_dependency_manager_t : public dependency_manager_t * relations that referenced deleted ways must be in the change file, too, * and so we don't have to find out which ones they are. */ - osmium::index::IdSetSmall m_changed_ways; + idlist_t m_changed_ways; /** * In append mode all new and changed relations will be added to this. * This is then used to remove already processed relations from the * pending list. */ - osmium::index::IdSetSmall m_changed_relations; + idlist_t m_changed_relations; - osmium::index::IdSetSmall m_ways_pending_tracker; - osmium::index::IdSetSmall m_rels_pending_tracker; + idlist_t m_ways_pending_tracker; + idlist_t m_rels_pending_tracker; }; #endif // OSM2PGSQL_DEPENDENCY_MANAGER_HPP diff --git a/src/idlist.cpp b/src/idlist.cpp new file mode 100644 index 000000000..7eec4f2b2 --- /dev/null +++ b/src/idlist.cpp @@ -0,0 +1,33 @@ +/** + * SPDX-License-Identifier: GPL-2.0-or-later + * + * This file is part of osm2pgsql (https://osm2pgsql.org/). + * + * Copyright (C) 2006-2023 by the osm2pgsql developer community. + * For a full list of authors see the git log. + */ + +#include "idlist.hpp" + +#include +#include +#include + +void idlist_t::sort_unique() +{ + std::sort(m_list.begin(), m_list.end()); + const auto last = std::unique(m_list.begin(), m_list.end()); + m_list.erase(last, m_list.end()); +} + +void idlist_t::merge_sorted(const idlist_t &other) +{ + std::vector new_list; + + new_list.reserve(m_list.size() + other.m_list.size()); + std::set_union(m_list.cbegin(), m_list.cend(), other.m_list.cbegin(), + other.m_list.cend(), std::back_inserter(new_list)); + + using std::swap; + swap(new_list, m_list); +} diff --git a/src/idlist.hpp b/src/idlist.hpp index 44a785564..a8523e820 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -38,8 +38,17 @@ class idlist_t auto end() const noexcept { return m_list.end(); } + auto cbegin() const noexcept { return m_list.cbegin(); } + + auto cend() const noexcept { return m_list.cend(); } + osmid_t operator[](std::size_t n) const noexcept { return m_list[n]; } + bool get_binary_search(osmid_t id) const noexcept + { + return std::binary_search(m_list.cbegin(), m_list.cend(), id); + } + void clear() { m_list.clear(); } void push_back(osmid_t id) { m_list.push_back(id); } @@ -64,6 +73,10 @@ class idlist_t return !(lhs == rhs); } + void sort_unique(); + + void merge_sorted(const idlist_t &other); + private: std::vector m_list; diff --git a/src/middle-pgsql.cpp b/src/middle-pgsql.cpp index 9b169ffdc..e441363e8 100644 --- a/src/middle-pgsql.cpp +++ b/src/middle-pgsql.cpp @@ -56,8 +56,7 @@ static bool check_bucket_index(pg_conn_t const *db_connection, } static void send_id_list(pg_conn_t const &db_connection, - std::string const &table, - osmium::index::IdSetSmall const &ids) + std::string const &table, idlist_t const &ids) { std::string data; for (auto const id : ids) { @@ -71,13 +70,12 @@ static void send_id_list(pg_conn_t const &db_connection, } static void load_id_list(pg_conn_t const &db_connection, - std::string const &table, - osmium::index::IdSetSmall *ids) + std::string const &table, idlist_t *ids) { auto const res = db_connection.exec( fmt::format("SELECT DISTINCT id FROM {} ORDER BY id", table)); for (int n = 0; n < res.num_tuples(); ++n) { - ids->set(osmium::string_to_object_id(res.get_value(n, 0))); + ids->push_back(osmium::string_to_object_id(res.get_value(n, 0))); } } @@ -827,10 +825,9 @@ void middle_pgsql_t::node_delete(osmid_t osm_id) } } -void middle_pgsql_t::get_node_parents( - osmium::index::IdSetSmall const &changed_nodes, - osmium::index::IdSetSmall *parent_ways, - osmium::index::IdSetSmall *parent_relations) const +void middle_pgsql_t::get_node_parents(idlist_t const &changed_nodes, + idlist_t *parent_ways, + idlist_t *parent_relations) const { util::timer_t timer; @@ -923,9 +920,8 @@ INSERT INTO osm2pgsql_changed_relations parent_ways->size(), parent_relations->size()); } -void middle_pgsql_t::get_way_parents( - osmium::index::IdSetSmall const &changed_ways, - osmium::index::IdSetSmall *parent_relations) const +void middle_pgsql_t::get_way_parents(idlist_t const &changed_ways, + idlist_t *parent_relations) const { util::timer_t timer; diff --git a/src/middle-pgsql.hpp b/src/middle-pgsql.hpp index 2e09e9b6b..701af4da6 100644 --- a/src/middle-pgsql.hpp +++ b/src/middle-pgsql.hpp @@ -24,6 +24,7 @@ #include #include "db-copy-mgr.hpp" +#include "idlist.hpp" #include "middle.hpp" #include "pgsql.hpp" @@ -117,14 +118,11 @@ struct middle_pgsql_t : public middle_t void after_ways() override; void after_relations() override; - void get_node_parents( - osmium::index::IdSetSmall const &changed_nodes, - osmium::index::IdSetSmall *parent_ways, - osmium::index::IdSetSmall *parent_relations) const override; + void get_node_parents(idlist_t const &changed_nodes, idlist_t *parent_ways, + idlist_t *parent_relations) const override; - void get_way_parents( - osmium::index::IdSetSmall const &changed_ways, - osmium::index::IdSetSmall *parent_relations) const override; + void get_way_parents(idlist_t const &changed_ways, + idlist_t *parent_relations) const override; class table_desc { diff --git a/src/middle.hpp b/src/middle.hpp index 00b323a7e..832550baf 100644 --- a/src/middle.hpp +++ b/src/middle.hpp @@ -10,12 +10,12 @@ * For a full list of authors see the git log. */ -#include #include #include #include +#include "idlist.hpp" #include "osmtypes.hpp" #include "thread-pool.hpp" @@ -149,16 +149,14 @@ class middle_t #endif } - virtual void get_node_parents( - osmium::index::IdSetSmall const & /*changed_nodes*/, - osmium::index::IdSetSmall * /*parent_ways*/, - osmium::index::IdSetSmall * /*parent_relations*/) const + virtual void get_node_parents(idlist_t const & /*changed_nodes*/, + idlist_t * /*parent_ways*/, + idlist_t * /*parent_relations*/) const { } - virtual void get_way_parents( - osmium::index::IdSetSmall const & /*changed_ways*/, - osmium::index::IdSetSmall * /*parent_relations*/) const + virtual void get_way_parents(idlist_t const & /*changed_ways*/, + idlist_t * /*parent_relations*/) const { } diff --git a/src/output-flex.cpp b/src/output-flex.cpp index 510746be0..030c0055a 100644 --- a/src/output-flex.cpp +++ b/src/output-flex.cpp @@ -998,7 +998,7 @@ void output_flex_t::select_relation_members() "integer way ids."}; } - m_stage2_way_ids->set(id); + m_stage2_way_ids->push_back(id); }); lua_pop(lua_state(), 2); // return value (a table), ways field (a table) @@ -1486,7 +1486,7 @@ void output_flex_t::init_lua(std::string const &filename) lua_remove(lua_state(), 1); // global "osm2pgsql" } -idset_t const &output_flex_t::get_marked_way_ids() +idlist_t const &output_flex_t::get_marked_way_ids() { if (m_stage2_way_ids->empty()) { log_info("Skipping stage 1c (no marked ways)."); diff --git a/src/output-flex.hpp b/src/output-flex.hpp index b64d15e13..d8e07d274 100644 --- a/src/output-flex.hpp +++ b/src/output-flex.hpp @@ -16,9 +16,9 @@ #include "flex-table-column.hpp" #include "flex-table.hpp" #include "geom.hpp" +#include "idlist.hpp" #include "output.hpp" -#include #include #include @@ -35,8 +35,6 @@ class geom_transform_t; class thread_pool_t; struct options_t; -using idset_t = osmium::index::IdSetSmall; - /** * When C++ code is called from the Lua code we sometimes need to know * in what context this happens. These are the possible contexts. @@ -128,7 +126,7 @@ class output_flex_t : public output_t void wait() override; - idset_t const &get_marked_way_ids() override; + idlist_t const &get_marked_way_ids() override; void reprocess_marked() override; void pending_way(osmid_t id) override; @@ -295,7 +293,7 @@ class output_flex_t : public output_t // This is shared between all clones of the output and must only be // accessed while protected using the lua_mutex. - std::shared_ptr m_stage2_way_ids = std::make_shared(); + std::shared_ptr m_stage2_way_ids = std::make_shared(); std::shared_ptr m_copy_thread; diff --git a/src/output.hpp b/src/output.hpp index 847dc3662..d380de3e6 100644 --- a/src/output.hpp +++ b/src/output.hpp @@ -16,8 +16,8 @@ * Common output layer interface. */ -#include - +#include "idlist.hpp" +#include "options.hpp" #include "osmtypes.hpp" #include "output-requirements.hpp" @@ -68,9 +68,9 @@ class output_t virtual void wait() {} - virtual osmium::index::IdSetSmall const &get_marked_way_ids() + virtual idlist_t const &get_marked_way_ids() { - static osmium::index::IdSetSmall const ids{}; + static idlist_t const ids{}; return ids; } From 3b7fdc5dc07954855b2589c6e462b0290f3a55db Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Sat, 19 Aug 2023 15:38:39 +0200 Subject: [PATCH 4/8] Remove unnecessary copy of idlist --- src/dependency-manager.cpp | 14 -------------- src/dependency-manager.hpp | 14 ++++++++++---- src/idlist.cpp | 2 +- src/idlist.hpp | 2 +- 4 files changed, 12 insertions(+), 20 deletions(-) diff --git a/src/dependency-manager.cpp b/src/dependency-manager.cpp index 534d42bfc..7ea433104 100644 --- a/src/dependency-manager.cpp +++ b/src/dependency-manager.cpp @@ -100,17 +100,3 @@ bool full_dependency_manager_t::has_pending() const noexcept { return !m_ways_pending_tracker.empty() || !m_rels_pending_tracker.empty(); } - -idlist_t full_dependency_manager_t::get_ids(idlist_t *tracker) -{ - tracker->sort_unique(); - - idlist_t list; - list.reserve(tracker->size()); - - std::copy(tracker->cbegin(), tracker->cend(), std::back_inserter(list)); - - tracker->clear(); - - return list; -} diff --git a/src/dependency-manager.hpp b/src/dependency-manager.hpp index e5271e89e..4902f1035 100644 --- a/src/dependency-manager.hpp +++ b/src/dependency-manager.hpp @@ -115,17 +115,23 @@ class full_dependency_manager_t : public dependency_manager_t idlist_t get_pending_way_ids() override { - return get_ids(&m_ways_pending_tracker); + idlist_t list; + using std::swap; + swap(list, m_ways_pending_tracker); + list.sort_unique(); + return list; } idlist_t get_pending_relation_ids() override { - return get_ids(&m_rels_pending_tracker); + idlist_t list; + using std::swap; + swap(list, m_rels_pending_tracker); + list.sort_unique(); + return list; } private: - static idlist_t get_ids(idlist_t *tracker); - std::shared_ptr m_object_store; /** diff --git a/src/idlist.cpp b/src/idlist.cpp index 7eec4f2b2..25a088613 100644 --- a/src/idlist.cpp +++ b/src/idlist.cpp @@ -20,7 +20,7 @@ void idlist_t::sort_unique() m_list.erase(last, m_list.end()); } -void idlist_t::merge_sorted(const idlist_t &other) +void idlist_t::merge_sorted(idlist_t const &other) { std::vector new_list; diff --git a/src/idlist.hpp b/src/idlist.hpp index a8523e820..c4828e503 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -75,7 +75,7 @@ class idlist_t void sort_unique(); - void merge_sorted(const idlist_t &other); + void merge_sorted(idlist_t const &other); private: std::vector m_list; From 3c1257e42e5b3497463143016b17ba3b2c5247be Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Sat, 19 Aug 2023 16:03:32 +0200 Subject: [PATCH 5/8] Replace set_diff function by something a bit more sensible --- src/dependency-manager.cpp | 19 ++----------------- src/idlist.cpp | 12 ++++++++++++ src/idlist.hpp | 6 ++++++ 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/src/dependency-manager.cpp b/src/dependency-manager.cpp index 7ea433104..3a600ff8a 100644 --- a/src/dependency-manager.cpp +++ b/src/dependency-manager.cpp @@ -39,27 +39,13 @@ void full_dependency_manager_t::after_nodes() m_changed_nodes.clear(); } -static idlist_t set_diff(idlist_t const &set, idlist_t const &to_be_removed) -{ - idlist_t new_set; - - for (auto const id : set) { - if (!to_be_removed.get_binary_search(id)) { - new_set.push_back(id); - } - } - - return new_set; -} - void full_dependency_manager_t::after_ways() { if (!m_changed_ways.empty()) { if (!m_ways_pending_tracker.empty()) { // Remove ids from changed ways in the input data from // m_ways_pending_tracker, because they have already been processed. - m_ways_pending_tracker = - set_diff(m_ways_pending_tracker, m_changed_ways); + m_ways_pending_tracker.remove_ids_if_in(m_changed_ways); // Add the list of pending way ids to the list of changed ways, // because we need the parents for them, too. @@ -83,8 +69,7 @@ void full_dependency_manager_t::after_relations() { // Remove ids from changed relations in the input data from // m_rels_pending_tracker, because they have already been processed. - m_rels_pending_tracker = - set_diff(m_rels_pending_tracker, m_changed_relations); + m_rels_pending_tracker.remove_ids_if_in(m_changed_relations); m_changed_relations.clear(); } diff --git a/src/idlist.cpp b/src/idlist.cpp index 25a088613..341ec4a6f 100644 --- a/src/idlist.cpp +++ b/src/idlist.cpp @@ -31,3 +31,15 @@ void idlist_t::merge_sorted(idlist_t const &other) using std::swap; swap(new_list, m_list); } + +void idlist_t::remove_ids_if_in(idlist_t const &other) +{ + std::vector new_list; + + new_list.reserve(m_list.size()); + std::set_difference(m_list.cbegin(), m_list.cend(), other.m_list.cbegin(), + other.m_list.cend(), std::back_inserter(new_list)); + + using std::swap; + swap(new_list, m_list); +} diff --git a/src/idlist.hpp b/src/idlist.hpp index c4828e503..518c18515 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -77,6 +77,12 @@ class idlist_t void merge_sorted(idlist_t const &other); + /** + * Remove all ids in this list that are also in the other list. Both + * lists must be sorted. + */ + void remove_ids_if_in(idlist_t const &other); + private: std::vector m_list; From 60987b4f15856185bb04329a1b46d62f360400fa Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Mon, 21 Aug 2023 09:36:25 +0200 Subject: [PATCH 6/8] Remove potentially expensive copy operator from idlist_t And fix the unnecessary copy this found. --- src/idlist.hpp | 7 +++++++ src/osmdata.cpp | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/idlist.hpp b/src/idlist.hpp index 518c18515..79919c1cc 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -27,9 +27,16 @@ class idlist_t using value_type = osmid_t; idlist_t() = default; + ~idlist_t() noexcept = default; idlist_t(std::initializer_list ids) : m_list(ids) {} + idlist_t(idlist_t const &) = delete; + idlist_t &operator=(idlist_t const &) = delete; + + idlist_t(idlist_t &&) = default; + idlist_t &operator=(idlist_t &&) = default; + bool empty() const noexcept { return m_list.empty(); } std::size_t size() const noexcept { return m_list.size(); } diff --git a/src/osmdata.cpp b/src/osmdata.cpp index 6051194b3..30e623e22 100644 --- a/src/osmdata.cpp +++ b/src/osmdata.cpp @@ -364,7 +364,7 @@ void osmdata_t::process_dependents() const } // stage 1c processing: mark parent relations of marked objects as changed - auto marked_ways = m_output->get_marked_way_ids(); + auto const &marked_ways = m_output->get_marked_way_ids(); if (marked_ways.empty()) { return; } From f082a359aaa70857d3c8adf7395db6806848d1ab Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Mon, 21 Aug 2023 09:59:18 +0200 Subject: [PATCH 7/8] Remove unused binary search function --- src/idlist.hpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/idlist.hpp b/src/idlist.hpp index 79919c1cc..72e98ba05 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -51,11 +51,6 @@ class idlist_t osmid_t operator[](std::size_t n) const noexcept { return m_list[n]; } - bool get_binary_search(osmid_t id) const noexcept - { - return std::binary_search(m_list.cbegin(), m_list.cend(), id); - } - void clear() { m_list.clear(); } void push_back(osmid_t id) { m_list.push_back(id); } From bde21d8ddbb3d72ee1ea6a8e184f6e63f09545ca Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Mon, 21 Aug 2023 10:00:10 +0200 Subject: [PATCH 8/8] Documentation and various code cleanups --- src/idlist.cpp | 9 +++++++++ src/idlist.hpp | 36 +++++++++++++++++++++++++----------- src/middle.hpp | 3 ++- src/pgsql-helper.cpp | 3 ++- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/src/idlist.cpp b/src/idlist.cpp index 341ec4a6f..d90ab0643 100644 --- a/src/idlist.cpp +++ b/src/idlist.cpp @@ -10,9 +10,18 @@ #include "idlist.hpp" #include +#include #include #include +osmid_t idlist_t::pop_id() +{ + assert(!m_list.empty()); + auto const id = m_list.back(); + m_list.pop_back(); + return id; +} + void idlist_t::sort_unique() { std::sort(m_list.begin(), m_list.end()); diff --git a/src/idlist.hpp b/src/idlist.hpp index 72e98ba05..e719f01a9 100644 --- a/src/idlist.hpp +++ b/src/idlist.hpp @@ -18,9 +18,14 @@ #include "osmtypes.hpp" -#include #include +/** + * A list of OSM object ids. Internally this is a vector of ids. + * + * Some operations are only allowed when the list of ids is sorted and + * without duplicates. Call sort_unique() to achieve this. + */ class idlist_t { public: @@ -51,20 +56,20 @@ class idlist_t osmid_t operator[](std::size_t n) const noexcept { return m_list[n]; } - void clear() { m_list.clear(); } + void clear() noexcept { m_list.clear(); } void push_back(osmid_t id) { m_list.push_back(id); } void reserve(std::size_t size) { m_list.reserve(size); } - osmid_t pop_id() - { - assert(!m_list.empty()); - auto const id = m_list.back(); - m_list.pop_back(); - return id; - } + /** + * Remove id at the end of the list and return it. + * + * \pre \code !m_list.empty()) \endcode + */ + osmid_t pop_id(); + /// List are equal if they contain the same ids in the same order. friend bool operator==(idlist_t const &lhs, idlist_t const &rhs) noexcept { return lhs.m_list == rhs.m_list; @@ -75,13 +80,22 @@ class idlist_t return !(lhs == rhs); } + /** + * Sort this list and remove duplicates. + */ void sort_unique(); + /** + * Merge other list into this one. + * + * \pre Both lists must be sorted and without duplicates. + */ void merge_sorted(idlist_t const &other); /** - * Remove all ids in this list that are also in the other list. Both - * lists must be sorted. + * Remove all ids in this list that are also in the other list. + * + * \pre Both lists must be sorted and without duplicates. */ void remove_ids_if_in(idlist_t const &other); diff --git a/src/middle.hpp b/src/middle.hpp index 832550baf..2edacfe5c 100644 --- a/src/middle.hpp +++ b/src/middle.hpp @@ -15,10 +15,11 @@ #include -#include "idlist.hpp" #include "osmtypes.hpp" #include "thread-pool.hpp" +class idlist_t; + struct options_t; struct output_requirements; diff --git a/src/pgsql-helper.cpp b/src/pgsql-helper.cpp index 8d367a6a5..b438594e4 100644 --- a/src/pgsql-helper.cpp +++ b/src/pgsql-helper.cpp @@ -14,8 +14,9 @@ #include idlist_t get_ids_from_result(pg_result_t const &result) { - idlist_t ids; assert(result.num_tuples() >= 0); + + idlist_t ids; ids.reserve(static_cast(result.num_tuples())); for (int i = 0; i < result.num_tuples(); ++i) {