From 8ce9bbe614aab043eb2a3b23edbce73f66266fa1 Mon Sep 17 00:00:00 2001 From: Jochen Topf Date: Tue, 11 May 2021 15:40:50 +0200 Subject: [PATCH] Improved node location store without need for freeze() Store ids and locations as they come in without going through a temporary block first. Slightly simpler code and we don't need the freeze() function any more. See also #1466. --- src/middle-ram.cpp | 2 - src/middle-ram.hpp | 2 - src/node-locations.cpp | 85 ++++++++++------------------------- src/node-locations.hpp | 35 +++++++-------- tests/test-node-locations.cpp | 4 -- 5 files changed, 39 insertions(+), 89 deletions(-) diff --git a/src/middle-ram.cpp b/src/middle-ram.cpp index 0f4397525..65b344a6d 100644 --- a/src/middle-ram.cpp +++ b/src/middle-ram.cpp @@ -184,8 +184,6 @@ void middle_ram_t::relation(osmium::Relation const &relation) } } -void middle_ram_t::after_nodes() { m_node_locations.freeze(); } - std::size_t middle_ram_t::nodes_get_list(osmium::WayNodeList *nodes) const { assert(nodes); diff --git a/src/middle-ram.hpp b/src/middle-ram.hpp index baa82cb37..d39af67e8 100644 --- a/src/middle-ram.hpp +++ b/src/middle-ram.hpp @@ -52,8 +52,6 @@ class middle_ram_t : public middle_t, public middle_query_t void way(osmium::Way const &way) override; void relation(osmium::Relation const &) override; - void after_nodes() override; - std::size_t nodes_get_list(osmium::WayNodeList *nodes) const override; bool way_get(osmid_t id, osmium::memory::Buffer *buffer) const override; diff --git a/src/node-locations.cpp b/src/node-locations.cpp index e5b47653e..20b2c7f5d 100644 --- a/src/node-locations.cpp +++ b/src/node-locations.cpp @@ -10,7 +10,6 @@ #include "node-locations.hpp" #include -#include // Workaround: This must be included before buffer_string.hpp due to a missing // include in the upstream code. https://github.com/mapbox/protozero/pull/104 @@ -24,13 +23,20 @@ void node_locations_t::set(osmid_t id, osmium::Location location) { - assert(block_index() == 0 || m_block[block_index() - 1].first < id); + if (first_entry_in_block()) { + m_did.clear(); + m_dx.clear(); + m_dy.clear(); + m_index.add(id, m_data.size()); + } + + protozero::add_varint_to_buffer(&m_data, m_did.update(id)); + protozero::add_varint_to_buffer( + &m_data, protozero::encode_zigzag64(m_dx.update(location.x()))); + protozero::add_varint_to_buffer( + &m_data, protozero::encode_zigzag64(m_dy.update(location.y()))); - m_block[block_index()] = {id, location}; ++m_count; - if (block_index() == 0) { - freeze(); - } } osmium::Location node_locations_t::get(osmid_t id) const @@ -46,38 +52,23 @@ osmium::Location node_locations_t::get(osmid_t id) const char const *const end = m_data.data() + m_data.size(); osmium::DeltaDecode did; - std::size_t num = block_size; - for (std::size_t n = 0; n < block_size; ++n) { - auto bid = did.update(protozero::decode_varint(&begin, end)); - if (bid == id) { - num = n; - } - if (bid > id && num == block_size) { - return osmium::Location{}; - } - } - if (num == block_size) { - return osmium::Location{}; - } - osmium::DeltaDecode dx; osmium::DeltaDecode dy; - int32_t x = 0; - int32_t y = 0; - for (std::size_t n = 0; n <= num; ++n) { - x = dx.update( + + for (std::size_t n = 0; n < block_size && begin != end; ++n) { + auto const bid = did.update(protozero::decode_varint(&begin, end)); + int32_t const x = dx.update( protozero::decode_zigzag64(protozero::decode_varint(&begin, end))); - y = dy.update( + int32_t const y = dy.update( protozero::decode_zigzag64(protozero::decode_varint(&begin, end))); + if (bid == id) { + return osmium::Location{x, y}; + } + if (bid > id) { + break; + } } - - return osmium::Location{x, y}; -} - -void node_locations_t::freeze() -{ - encode_block(); - clear_block(); + return osmium::Location{}; } void node_locations_t::clear() @@ -85,33 +76,5 @@ void node_locations_t::clear() m_data.clear(); m_data.shrink_to_fit(); m_index.clear(); - clear_block(); m_count = 0; } - -void node_locations_t::encode_block() -{ - auto const offset = m_data.size(); - osmium::DeltaEncode did; - osmium::DeltaEncode dx; - osmium::DeltaEncode dy; - for (auto const &nl : m_block) { - protozero::add_varint_to_buffer(&m_data, did.update(nl.first)); - } - for (auto const &nl : m_block) { - protozero::add_varint_to_buffer( - &m_data, protozero::encode_zigzag64(dx.update(nl.second.x()))); - protozero::add_varint_to_buffer( - &m_data, protozero::encode_zigzag64(dy.update(nl.second.y()))); - } - m_index.add(m_block[0].first, offset); -} - -void node_locations_t::clear_block() -{ - for (auto &nl : m_block) { - nl.first = std::numeric_limits::max(); - nl.second = osmium::Location{}; - } -} - diff --git a/src/node-locations.hpp b/src/node-locations.hpp index 520b0bba1..47e547193 100644 --- a/src/node-locations.hpp +++ b/src/node-locations.hpp @@ -13,6 +13,8 @@ #include "ordered-index.hpp" #include "osmtypes.hpp" +#include + #include #include #include @@ -25,18 +27,14 @@ * * Internally nodes are stored in blocks of `block_size` (id, location) pairs. * Ids inside a block and the x and y coordinates of each location are first - * delta encoded and then stored as varints. To access a stored location a - * full block must be decoded. + * delta encoded and then stored as varints. To access a stored location the + * block must be decoded until the id is found. * - * Ids must be added in strictly ascending order. After all ids are stored, - * the `freeze()` function must be called. Only after that can the store - * be queried. + * Ids must be added in strictly ascending order. */ class node_locations_t { public: - node_locations_t() { clear_block(); } - /** * Store a node location. * @@ -60,22 +58,16 @@ class node_locations_t } /** - * Freeze storage. Muste be called after set()ing all the ids and before - * get()ing the first one. - */ - void freeze(); - - /** - * Clear the memory used by this object. The object can not be reused - * after that. + * Clear the memory used by this object. The object can be reused after + * that. */ void clear(); private: - std::size_t block_index() const noexcept { return m_count % block_size; } - - void encode_block(); - void clear_block(); + bool first_entry_in_block() const noexcept + { + return m_count % block_size == 0; + } /** * The block size used for internal blocks. The larger the block size @@ -83,12 +75,15 @@ class node_locations_t */ static constexpr const std::size_t block_size = 32; - std::array, block_size> m_block; ordered_index_t m_index; std::string m_data; /// The number of (id, location) pairs stored. std::size_t m_count = 0; + + osmium::DeltaEncode m_did; + osmium::DeltaEncode m_dx; + osmium::DeltaEncode m_dy; }; // class node_locations_t #endif // OSM2PGSQL_NODE_LOCATIONS_HPP diff --git a/tests/test-node-locations.cpp b/tests/test-node-locations.cpp index 9db6a6c77..1358503c3 100644 --- a/tests/test-node-locations.cpp +++ b/tests/test-node-locations.cpp @@ -21,9 +21,6 @@ TEST_CASE("node locations basics", "[NoDB]") REQUIRE(nl.size() == 2); - nl.freeze(); - REQUIRE(nl.size() == 2); - REQUIRE(nl.get(1) == osmium::Location{}); REQUIRE(nl.get(4) == osmium::Location{}); REQUIRE(nl.get(6) == osmium::Location{}); @@ -70,7 +67,6 @@ TEST_CASE("node locations in more than one block", "[NoDB]") nl.set(id, {id + 0.1, id + 0.2}); } - nl.freeze(); REQUIRE(nl.size() == max_id); for (std::size_t id = 1; id <= max_id; ++id) {