Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

mugr1x · 2023-04-03T19:43:45Z

Issue

Since C++17 added support for 'containers', #6551 targets at replacing current code that uses boost::optional to use std::optional. This PR replaces some parts of the codebase with std::optional, indeed the ones I could convert without having to rewrite some parsers, see #6592 . I try complete these parser, but without that any further migrations to std::optional cause only myriads of compiler errors.

Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages ==> none edited
add tests (see testing documentation) ==> no new functions to be tested
review
adjust for comments
cherry pick to release branch

Requirements / Relations

#6551
#6592

mugr1x · 2023-04-03T21:33:31Z

Tests fail with segfault.

mugr1x · 2023-04-03T22:55:37Z

Had something to do with DecreaseKey in query_heap.hpp.
EDIT: Sorry had submitted the wrong commit yesterday (i.e. the one that segfaults, I got around this by calling in DecreaseKey not boost::heap::increase as current master but boost::heap::decrease, hope this is right)
Regarding the errorrs in CI, are these formatting errors ? I ran scripts/format.sh as advised.

mugr1x · 2023-04-10T17:45:45Z

Relevant documentation includes:
https://www.boost.org/doc/libs/1_76_0/doc/html/boost/heap/d_ary_heap.html#id-1_3_18_6_2_1_1_1_22_17-bb

SiarheiFedartsou · 2023-04-14T14:28:51Z

include/contractor/contract_excludable_graph.hpp

-                std::remove_if(non_core_edges.begin(), non_core_edges.end(), [&](const auto &edge) {
-                    return is_shared_core[edge.source] && is_shared_core[edge.target];
-                });
+            auto new_end = std::remove_if(non_core_edges.begin(),


Hm, do I get it correctly that most of changes in this PR are format changes. Were they pushed by mistake?

Better now ?

SiarheiFedartsou · 2023-04-14T19:23:54Z

include/util/query_heap.hpp

@@ -289,7 +289,7 @@ class QueryHeap
        return inserted_nodes[index].node == node;
    }

-    boost::optional<HeapNode &> GetHeapNodeIfWasInserted(const NodeID node)
+    std::optional<HeapNode> GetHeapNodeIfWasInserted(NodeID node)


I.e. now on each call we will copy HeapNode, right? How do we know that it won’t impact performance? May be worth refactoring it to HeapNode* instead(I mean use just pointer instead of optional here)? From some perspective optional<T&> is just overcomplicated way to express simple pointer…

WDYT?

P.S. I didn’t have a time yet to look over the rest of PR, just something important what I noticed…

More than it: here you can see an example when we change contents of node in a heap and that by reference…

osrm-backend/include/engine/routing_algorithms/routing_base_mld.hpp

Line 331 in d516314

toHeapNode->data = {heapNode.node, true};

I.e. if we return it not by reference now most likely existing logic is broken.

I did run my build and it did at least not segfault or got killed by kernel for too much RAM.
To cite from cppreference.com:
"There are no optional references; a program is ill-formed if it instantiates an optional with a reference type."
Search engine would suggest std::reference_wrapper for this task.

At the same time as I can see if we just change it to simple pointer instead of optional(I.e. won’t use neither boost::optional nor std::optional) the rest of code which uses this QueryHeap would “just work” without changes we had to make there…

Regarding the other comment std::optional defines an operator->. However in some cases you probably would need to change a deref etc. I'm just testing solely using pointers for these parts. Does not work out of the box, but the idea should be doable.

boost::optional<T&> seems to be effectively the same as T*, isn’t it?

Yes it is. Still forking the main branch I need replace some of the idioms. Do you prefer nullptr or some void coercion ?

What is void coercion? You mean if (ptr == nullptr) vs if (!ptr)? Personally I like first option as more explicit, but I don’t think it would be blocker to merge this PR :)

Actually it's dreadfully simple, memory safety should be fine since it tests. I got too fed up with the nullptr version.
I think you need to approve CI again.

danpat · 2023-04-14T21:54:17Z

include/util/query_heap.hpp

@@ -356,13 +356,13 @@ class QueryHeap
        const auto index = node_index.peek_index(node);
        auto &reference = inserted_nodes[index];
        reference.weight = weight;
-        heap.increase(reference.handle, std::make_pair(weight, index));
+        heap.decrease(reference.handle, std::make_pair(weight, index));


This change is pretty fundamental to how the whole routing algorithm works. Can you explain exactly why this change is being made here?

See my previous link posted above:
https://www.boost.org/doc/libs/1_76_0/doc/html/boost/heap/d_ary_heap.html#id-1_3_18_6_2_1_1_1_22_17-bb
My thoughts behind this are that there is logic in multiple files e.g. include/engine/routing_algorithms/routing_base_mld.hpp that checks for a certain "to_weight" to be smaller than the current "toHeapNode->weight" and then calls DecreaseKey which in turn calls heap.increase with "to_weight".
So the docs say: "The new value is expected to be less than the current one" for decrease not increase.
Also I guess it kinda fixed a segfault for me while testing. (I know for sure it was this one, however might only have emerged with now reverted changes since now not using std::optional instead just pointers)
Am I completely mistaken here ? I still get from A to B though using my setup e.g. with the Germany map.

danpat

Can you explain why the change from heap.increase() to heap.decrease() ? "The old version was wrong because XYZ?" is an acceptable answer, but this is a pretty fundamental change to a core part of the routing algorithm, I think it needs a clear rationale.

mugr1x · 2023-04-15T18:07:44Z

@SiarheiFedartsou @danpat
Can anyone please approve CI again ?

danpat · 2023-04-16T21:39:22Z

@mugr1x You didn't address my question about the changes in DecreaseKey in query_heap.hpp - why are those changes included? It's not obvious to me why changing to returning a pointer involves switching from .increase() to .decrease().

mugr1x · 2023-04-17T08:26:13Z

Well I linked the docs two times and explained my reasoning. I opened a separate issue #6603 for this but I would really advise you too look into this anyway soonish. As said I fixed a segfault at some point for me.
Also you are completely right it has nothing to do with just returning a pointer in GetHeapNodeIfWasInserted, however this is the same in master, because as Siarhei pointed out boost::optional<T&> is just a disguise for T*.
So the function always returned a pointer nothing is changed by me except the idiom.

SiarheiFedartsou · 2023-04-17T08:29:59Z

Well I linked the docs two times and explained my reasoning. I opened a separate issue #6603 for this but I would really advise you too look into this anyway soonish.

Can we discuss that in separate PR? This change is serious enough to deserve separate PR... Also if you see that it doesn't work in certain cases it is always a good practice to add test for such case to not regress it in the future.

mugr1x · 2023-04-17T08:34:19Z

Well I linked the docs two times and explained my reasoning. I opened a separate issue #6603 for this but I would really advise you too look into this anyway soonish.

Can we discuss that in separate PR? This change is serious enough to deserve separate PR... Also if you see that it doesn't work in certain cases it is always a good practice to add test for such case to not regress it in the future.

Yes

mugr1x · 2023-04-17T08:38:11Z

I just noted that PR #6596 was merged, I had the same fix as that in this PR too. So I just resolved this one with master.

…ject-OSRM#6596)

AlTimofeyev · 2023-04-18T08:51:52Z

Hi @mugr1x , I took a glance at that error you mentioned and it looks like the file where block_counter is mentioned in your PR is redefined. Take a glance at your commits. File range_table.hpp has two definitions for that variable on lines 84 and 86. It must've happened when you resolved with master (my bad about that).

mugr1x · 2023-04-18T08:59:54Z

@danpat Are you happy with the current status ? If so can you close the review or similar.
@AlTimofeyev Yes, I first merged my own and your fix. You are right. I just did look at my local source not the the diff.
Thank you very much.

mugr1x · 2023-04-19T02:09:31Z

CI shows exactly the same error about block_counter, which is weird since it is clearly only defined once now, also in diff.
Probably this has nothing to do with this PR, so merge anyway ?

SiarheiFedartsou · 2023-04-19T06:58:03Z

@mugr1x may I ask you to create one more empty commit to trigger CI again? I suspect it is some bug in GitHub Actions.

danpat · 2023-04-19T22:21:01Z

include/util/range_table.hpp

        BlockT block;
 #ifndef BOOST_ASSERT_IS_VOID
+        unsigned block_counter = 0;


These changes were merged to master in another PR #6596 - they should not also be showing up in this PR. I suspect some merges have been incorrectly applied or lost on the branch. This is probably also why CI is failing. I suspect if you tidy this up, CI should pass.

The source code on master branch has block_counter = 0 on line 85, meanwhile in this commit it is placed on line 84. Could that be what's causing CI to fail?

@mugr1x may be try to squash all your commits into single one? Or open another PR? Tbh for me it is not clear why it is happening - looks more like some GH bug.

@AlTimofeyev Well yes. Idk that much about CI.
@SiarheiFedartsou I did so #6611. Should I close this one ?
I also tried to rewrite some parsers. Point here is that the ones that are easy to rewrite work fine with switching to std::optional but the /server core API (and /engine because it there are crosslinks) don't.
I think this would be considered a too breaking change (at least ~1k lines of code) of new parser-related code.
Do you think there is value in #6592 or should I close this too ?
Also off-topic, but did you receive my email regarding GSoC. I was asking for what you mentors would expect. (I submitted my second version a bit late, though in time. Hope that is not a problem.)

mugr1x · 2023-04-24T15:08:13Z

I just realized I had a lot of comments show as 'Pending', which means appearantly noone but me could seee those.
Anyway probably I should close this once the other PR is merged.

mugr1x closed this Apr 3, 2023

mugr1x reopened this Apr 3, 2023

mugr1x force-pushed the boost_optional branch 2 times, most recently from 328eaaf to 97d3fcb Compare April 4, 2023 09:50

Mugr Rex added 4 commits April 4, 2023 14:06

Replace some boost::optional with std::optional

97d3fcb

Updated guidance/

850a3a7

Updated nodejs/

81d6e1f

Fix compiler error in include/util/range_table.hpp

bb58c9d

SiarheiFedartsou reviewed Apr 14, 2023

View reviewed changes

mugr1x force-pushed the boost_optional branch 2 times, most recently from 1fc8d4c to f98d73c Compare April 14, 2023 18:37

SiarheiFedartsou reviewed Apr 14, 2023

View reviewed changes

Reformatted using clang-format-10

f98d73c

mugr1x force-pushed the boost_optional branch 2 times, most recently from 54ef1b3 to fb0400f Compare April 14, 2023 21:21

danpat reviewed Apr 14, 2023

View reviewed changes

danpat requested changes Apr 14, 2023

View reviewed changes

Mugr Rex added 2 commits April 15, 2023 01:31

Switched to not using std::optional for query_heap.hpp

fb0400f

Comment fix

c9ac8ee

Mugr Rex and others added 2 commits April 15, 2023 21:58

Resolve conflicts with Project-OSRM#6602

e920bbb

Merge branch 'master' into boost_optional

ec74be3

mugr1x force-pushed the boost_optional branch from 392c085 to 7f6900d Compare April 17, 2023 08:30

Mugr Rex and others added 2 commits April 17, 2023 12:47

Reverted changes in query_heap.hpp

7f6900d

Added block_counter variable to a preprocessor check, minor bug. (Pro…

c42d326

…ject-OSRM#6596)

Reverted one range_table.hpp fix

9242365

SiarheiFedartsou requested a review from danpat April 18, 2023 21:17

Make GitHub and CI happy

f54aa0a

danpat reviewed Apr 19, 2023

View reviewed changes

mugr1x mentioned this pull request Apr 20, 2023

Replace boost::optional with std::optional #6611

Merged

mugr1x force-pushed the boost_optional branch from 2ef11fb to f54aa0a Compare April 20, 2023 20:33

mugr1x closed this Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

mugr1x commented Apr 3, 2023

mugr1x commented Apr 3, 2023

mugr1x commented Apr 3, 2023 •

edited

Loading

mugr1x commented Apr 10, 2023

SiarheiFedartsou Apr 14, 2023

mugr1x Apr 14, 2023

SiarheiFedartsou Apr 14, 2023

SiarheiFedartsou Apr 14, 2023

mugr1x Apr 14, 2023

SiarheiFedartsou Apr 14, 2023

mugr1x Apr 14, 2023

SiarheiFedartsou Apr 14, 2023

mugr1x Apr 14, 2023

SiarheiFedartsou Apr 14, 2023

mugr1x Apr 14, 2023

danpat Apr 14, 2023

mugr1x Apr 14, 2023

danpat left a comment

mugr1x commented Apr 15, 2023

danpat commented Apr 16, 2023

mugr1x commented Apr 17, 2023

SiarheiFedartsou commented Apr 17, 2023

mugr1x commented Apr 17, 2023

mugr1x commented Apr 17, 2023

AlTimofeyev commented Apr 18, 2023

mugr1x commented Apr 18, 2023 •

edited

Loading

mugr1x commented Apr 19, 2023 •

edited

Loading

SiarheiFedartsou commented Apr 19, 2023

danpat Apr 19, 2023 •

edited

Loading

AlTimofeyev Apr 20, 2023 •

edited

Loading

SiarheiFedartsou Apr 20, 2023

mugr1x Apr 20, 2023

mugr1x commented Apr 24, 2023

Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

Refactor: Replace parts of boost::optional with std::optional (#6551,… #6593

Conversation

mugr1x commented Apr 3, 2023

Issue

Tasklist

Requirements / Relations

mugr1x commented Apr 3, 2023

mugr1x commented Apr 3, 2023 • edited Loading

mugr1x commented Apr 10, 2023

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

danpat left a comment

Choose a reason for hiding this comment

mugr1x commented Apr 15, 2023

danpat commented Apr 16, 2023

mugr1x commented Apr 17, 2023

SiarheiFedartsou commented Apr 17, 2023

mugr1x commented Apr 17, 2023

mugr1x commented Apr 17, 2023

AlTimofeyev commented Apr 18, 2023

mugr1x commented Apr 18, 2023 • edited Loading

mugr1x commented Apr 19, 2023 • edited Loading

SiarheiFedartsou commented Apr 19, 2023

danpat Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

AlTimofeyev Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mugr1x commented Apr 24, 2023

mugr1x commented Apr 3, 2023 •

edited

Loading

mugr1x commented Apr 18, 2023 •

edited

Loading

mugr1x commented Apr 19, 2023 •

edited

Loading

danpat Apr 19, 2023 •

edited

Loading

AlTimofeyev Apr 20, 2023 •

edited

Loading