Split update code into own module and refactor it #3785

TheMarex · 2017-03-07T23:43:10Z

Issue

Splits out the update code into an own module in preparation to be used in the customizer as well as in the contractor. ~~Aim is to implement #3737~~

Update: Implementing issue #3737 would require a few invasive changes that I don't want to include in this PR since it moves a lot of code. Splitting this off in a separate PR.

Tasklist

Split update code from osrm-contract
Clean up updater
~~Implement update method based on Refactor segment update code #3737~~
review
adjust for comments

Requirements / Relations

~~This is need for #3782 because the current update code assumes the edges are in the same order as in the osrm-extract code.~~

daniel-j-h · 2017-03-09T16:24:14Z

include/extractor/compressed_edge_container.hpp

@@ -58,21 +59,19 @@ class CompressedEdgeContainer
    NodeID GetLastEdgeTargetID(const EdgeID edge_id) const;
    NodeID GetLastEdgeSourceID(const EdgeID edge_id) const;

+    // Invalidates the internal storage
+    SegmentDataContainer ToSegmentData();


Urgh this is a bit weird - can you do the following in the function impl.

void fn() { static bool dead = false; assert !dead; dead = true; return move(data); }

Hrm I can put it in a unique_ptr, I guess that is what it would be for.

daniel-j-h · 2017-03-09T16:25:13Z

include/extractor/io.hpp

+{
+
+template <>
+void read(const boost::filesystem::path &path, SegmentDataContainer &segment_data)


Why is this a template (specialization)?

Because SegmentDataContainer = detail::SegmentDataContainer<UseSharedMemory=false>. This could extended to allow the shared memory version, I'm not sure this makes sense though.

daniel-j-h · 2017-03-09T16:28:33Z

include/extractor/io.hpp

+    writer.WriteFrom(segment_data.fwd_weights.data(), segment_data.fwd_weights.size());
+    writer.WriteFrom(segment_data.rev_weights.data(), segment_data.rev_weights.size());
+    writer.WriteFrom(segment_data.fwd_durations.data(), segment_data.fwd_durations.size());
+    writer.WriteFrom(segment_data.rev_durations.data(), segment_data.rev_durations.size());


Hm here and above - we should probably provide a range-based read/write function for contiguous containers. Pointer and size is such a common pattern, this all should be

reader.ReadInto(myvec); writer.WriteFrom(myvec);

Actually we already had that for reading, added it for writing too.

We already have FileWriter::SerializeVector() and FileReader::DeserializeVector() those should probably be used here. Maybe not the best names, but they're already used elsewhere, so we should stay consistent until we have a better interface.

daniel-j-h · 2017-03-09T16:31:21Z

include/extractor/segment_data_container.hpp

+        return rev_weights[index[id] + offset];
+    }
+    // TODO we only need this for the datasource file since it breaks this
+    // abstraction, but uses this index


Want to ticket al TODOs?

Captured in #3797

daniel-j-h · 2017-03-09T16:32:26Z

include/extractor/segment_data_container.hpp

+}
+
+using SegmentDataView = detail::SegmentDataContainerImpl<true>;
+using SegmentDataContainer = detail::SegmentDataContainerImpl<false>;


Hm generating two types for this is a bit weird but I can see how it has to be done for the shm vec...

Btw what we should do (not only here) is to stick to enums and specialize based on the enums instead of true / false.

SegmentDataContainerImpl<false> // ?? what is false? I have to look this up SegmentDataContainerImpl<DoNotUseSharedMemory> // ah!

I agree, we should do this in a sweep that changes it everywhere consistently.

#3799

daniel-j-h · 2017-03-09T16:41:01Z

include/updater/updater.hpp

+
+    EdgeID LoadAndUpdateEdgeExpandedGraph(
+        std::vector<extractor::EdgeBasedEdge> &edge_based_edge_list,
+        std::vector<EdgeWeight> &node_weights) const;


Hm should this be split into loading (can be re-used) and updating?

Also from looking at the interface only what is the return edge id supposed to mean? max id I guess but it's not clear from the decl.

Yeah I'm not done with the refactor, there will be more splits here. Just wanted to get the big move into master. 👍

daniel-j-h · 2017-03-09T16:42:24Z

include/updater/updater.hpp

+        std::vector<EdgeWeight> &node_weights) const;
+
+private:
+    UpdaterConfig config;


Storing by const ref? Otherwise take by value and move in. Because now a ocpy is always made. If you take a value and move in users can move in, too. No copy is made.

daniel-j-h · 2017-03-09T16:43:12Z

include/util/shared_memory_vector_wrapper.hpp

@@ -34,6 +34,7 @@ class ShMemIterator
    typedef typename base_t::reference reference;
    typedef std::random_access_iterator_tag iterator_category;

+    explicit ShMemIterator() : m_value(nullptr) {}


Hm why the default ctor now?

We need this for the boost::adapters::reverse range.

daniel-j-h · 2017-03-09T16:46:44Z

src/extractor/compressed_edge_container.cpp

+
+SegmentDataContainer CompressedEdgeContainer::ToSegmentData()
+{
+    return std::move(segment_data);


assert not dead

daniel-j-h · 2017-03-09T16:47:17Z

src/updater/csv_source.cpp

+                                 via)(decltype(osrm::updater::Turn::to), to))
+BOOST_FUSION_ADAPT_STRUCT(osrm::updater::PenaltySource,
+                          (decltype(osrm::updater::PenaltySource::duration),
+                           duration)(decltype(osrm::updater::PenaltySource::weight), weight))


urgh format seems to be broken here, what about

// clang-format off manually format // clang-format on

oxidase

👍

oxidase · 2017-03-10T06:46:56Z

src/updater/updater.cpp

+    if (std::isfinite(weight))
+        return std::round(weight * weight_multiplier);
+
+    return duration == MAXIMAL_EDGE_DURATION ? INVALID_EDGE_WEIGHT


Here is a regression that i personally missed: traffic updates without weights will silently change to the "duration" weight type, so somehow this should be made explicit with correct decision:

assertion profile_properties.IsFallbackToDurationAllowed()

returning INVALID_EDGE_WEIGHT if !profile_properties.IsFallbackToDurationAllowed()

or make weights field in CSV files required

ATM it messes up updated edge weights when they are different wrt "duration" weights.

We have the problem however that we depend on this functionality right now on our traffic processing. This is not a real problem for us right, since our weights are duration based still anyway.

We should re-think the whole approach on how we ingest weight/duration data though. It is much too easy to screw this up. Can you ticket this?

oxidase · 2017-03-10T06:50:43Z

src/updater/updater.cpp

+    }
+}
+
+void CheckWeightsConsistency(


Please move the function in #if !defined(NDEBUG) .. #endif to avoid unused function warnings

oxidase · 2017-03-10T06:51:35Z

src/updater/updater.cpp

+    storage::io::FileReader profile_properties_file(config.profile_properties_path,
+                                                    storage::io::FileReader::HasNoFingerprint);
+    profile_properties_file.ReadInto<extractor::ProfileProperties>(&profile_properties, 1);
+    weight_multiplier = profile_properties.GetWeightMultiplier();


auto weight_multiplier = profile_properties.GetWeightMultiplier(); and remove L167

oxidase · 2017-03-10T07:10:59Z

include/updater/updater_config.hpp

@@ -0,0 +1,81 @@
+/*
+
+Copyright (c) 2016, Project OSRM contributors


Just curious about license boilerplate, would be enough just to reference LICENSE.txt?

I think we removed it everywhere except for public headers we install. But even then it's questionable..

Also it's 2017 already, who wants to s/2016/2017/g ? :D

oxidase · 2017-03-10T07:17:21Z

include/extractor/io.hpp

+
+    // FIXME this _should_ just be size and the senitel below need to be removed
+    writer.WriteElementCount32(segment_data.index.size() + 1);
+    writer.WriteFrom(segment_data.index.data(), segment_data.index.size());


writer.WriteFrom(segment_data.index);

oxidase · 2017-03-10T07:20:08Z

include/extractor/io.hpp

+    writer.WriteElementCount32(segment_data.index.size() + 1);
+    writer.WriteFrom(segment_data.index.data(), segment_data.index.size());
+    // FIMXE remove unnecessary senitel
+    writer.WriteElementCount32(segment_data.nodes.size());


why not remove it now? it will require only segment_data.index.push_back(num_entries) // add sentinel on L25

Right, originally I didn't want to break the data format but I'm doing this with #3797 anyway.

Actually I found the real reason the code was added at the time: The way CompressedEdgeContainer constructs the index was broken, so somehow a fix for this landed in the IO layer. Fixed now. 👍

TheMarex · 2017-03-10T11:06:12Z

@daniel-j-h @oxidase pushed some fixes to address your PR comments. Please let me know if that works for you and merge this once travis gives it a 🍏 .

TheMarex added MLD Work In Progress labels Mar 7, 2017

TheMarex force-pushed the refactor/update branch 6 times, most recently from 20a406b to f044be7 Compare March 9, 2017 15:21

TheMarex added 4 commits March 9, 2017 15:27

Split updater code from contract into own module

6964e38

Split CSV parsing into nicer interface

405e5c1

Refactor compressed geometry in own abstraction with read/write

e82b79c

Consolidate read/write code in updater for compressed geometries

a4da1a7

TheMarex force-pushed the refactor/update branch from f044be7 to a4da1a7 Compare March 9, 2017 15:28

TheMarex added Review and removed Work In Progress labels Mar 9, 2017

TheMarex mentioned this pull request Mar 9, 2017

Fold datasource into SegmentDataContainer #3797

Closed

daniel-j-h reviewed Mar 9, 2017

View reviewed changes

TheMarex mentioned this pull request Mar 9, 2017

Refactor code that handles datasources #3800

Merged

3 tasks

TheMarex added 3 commits March 9, 2017 22:44

Address PR comments

33d63b9

Apply clang-format

848b0e9

Simplify write/read code

e4989fd

oxidase requested changes Mar 10, 2017

View reviewed changes

TheMarex mentioned this pull request Mar 10, 2017

Integrate traffic update functionality in osrm-customize #3803

Closed

Address PR comment by @oxidase

8e02027

TheMarex added Review - In feedback and removed Review labels Mar 10, 2017

oxidase approved these changes Mar 10, 2017

View reviewed changes

TheMarex merged commit ffd6311 into master Mar 10, 2017

TheMarex deleted the refactor/update branch March 10, 2017 14:43

TheMarex mentioned this pull request Mar 11, 2017

Refactor update routines #3809

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split update code into own module and refactor it #3785

Split update code into own module and refactor it #3785

TheMarex commented Mar 7, 2017 •

edited

Loading

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017 •

edited

Loading

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

danpat Mar 10, 2017

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

daniel-j-h Mar 9, 2017

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

daniel-j-h Mar 9, 2017

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

daniel-j-h Mar 9, 2017

daniel-j-h Mar 9, 2017

TheMarex Mar 9, 2017

daniel-j-h Mar 9, 2017

daniel-j-h Mar 9, 2017

oxidase left a comment

oxidase Mar 10, 2017

TheMarex Mar 10, 2017

oxidase Mar 10, 2017

oxidase Mar 10, 2017

oxidase Mar 10, 2017

oxidase Mar 10, 2017

daniel-j-h Mar 10, 2017

oxidase Mar 10, 2017

oxidase Mar 10, 2017

TheMarex Mar 10, 2017

TheMarex Mar 10, 2017

TheMarex commented Mar 10, 2017

		@@ -0,0 +1,81 @@
		/*

		Copyright (c) 2016, Project OSRM contributors

Split update code into own module and refactor it #3785

Split update code into own module and refactor it #3785

Conversation

TheMarex commented Mar 7, 2017 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

TheMarex Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxidase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMarex commented Mar 10, 2017

TheMarex commented Mar 7, 2017 •

edited

Loading

TheMarex Mar 9, 2017 •

edited

Loading