Issue456

Title use shared_ptr for AbstractTask
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte, pvonreth, silvan
Assigned To pvonreth Keywords
Optional summary

Created on 2014-08-23.01:09:05 by jendrik, last changed by jendrik.

Messages
msg4178 (view) Author: jendrik Date: 2015-04-22.11:05:59
I took the liberty to sign off on this myself. Code is merged and pushed. Thanks 
Patrick!
msg4177 (view) Author: jendrik Date: 2015-04-21.16:29:48
regarding the lower case conversion: I think we should restore the lower-casing since it is 
unrelated to this issue. The const references can stay where they make sense.

I talked to Malte about the pointers vs. references issue and we agreed that copy assignment 
is useful. Therefore, we will keep the raw pointers in all proxy classes.
msg4176 (view) Author: jendrik Date: 2015-04-21.15:57:09
regarding pointers vs. references: What I meant was the copy assignment operator. Currently, we do not need the copy assignment operator anywhere 
in the master repo, but I needed it for implementing swap-and-pop for vectors of <Operator,AbstractState> pairs in the CEGAR branch. The only 
class in the master repo that needs the raw pointer member is State. There we need it to reset other.task to nullptr in the move constructor. The 
question is whether these two use cases really need the raw pointer. If they do, the question becomes whether we want all Proxy classes to store 
the AbstractTask in the same way or not.
msg4175 (view) Author: pvonreth Date: 2015-04-21.15:28:57
regarding the to_lower:
the copy of the key was mad lowercase and the object then inserted with the 
lower case key. but on retrieval to_lower was not executed. The inconsistency 
could cause problems. But I'm fine with reverting that change if it was really 
intended that way.
msg4174 (view) Author: malte Date: 2015-04-21.15:17:30
Regarding the code review: I won't have time for this. If someone else is happy
to sign off on this, feel free to merge.
msg4173 (view) Author: malte Date: 2015-04-21.15:13:55
Regarding to_lower: as long as what works currently keeps working, it's less
important how this is internally achieved in the option parser. I just want to
make sure that we don't break the upper-case to lower-case conversions we
currently need.

For example, this concerns enum options like "HIGH" and "LOW" in shrink_fh,
where we use upper-case in the code but the user should be able to user
lower-case, or the fact that things like --search "ASTAR(BLIND())" should
continue to work. "Never converting to lower case" rang some alarms there.
msg4172 (view) Author: pvonreth Date: 2015-04-21.15:03:15
to lower was only used when a key was registered. but never on contains and get
https://bitbucket.org/TheOneRing/hg.fast-downward.org/pull-
request/10/shared_ptr/diff#Lsrc/search/option_parser_util.hF97
msg4171 (view) Author: malte Date: 2015-04-21.14:07:40
> and saving the AbstractTask as a raw pointer instead of as a reference in the
> proxy classes to enable their default copy constructors

Copy constructors work just as well with references as with pointers. Is there
another underlying reason? Did you perhaps mean "default constructor"?

Allowing these classes to have a default constructor would be a mis-feature, I
think. If we can at all avoid it that would be good because making sure all
objects are in a sane state is one of the main principles in object-oriented
design, and a proxy object without a valid abstract task to refer to isn't in a
valid state.

> Also, we now ... never convert the strings to lower case.

I'm not sure which strings we're talking about here, but this sounds unrelated
and potentially undesirable. Our option parser was intentionally meant to be
case-insensitive, and converting to a canonical form such as lower case is one
of the standard implementation strategies for something like this.
msg4170 (view) Author: jendrik Date: 2015-04-21.11:24:27
The first step is almost done: the parser now returns shared_ptr<AbstractTask> instead of a raw pointer to 
AbstractTask. Additionally, we cleaned up those parts of the code we had to touch anyway. This includes 
some C++11 changes and saving the AbstractTask as a raw pointer instead of as a reference in the proxy 
classes to enable their default copy constructors. Also, we now pass strings as const references to the 
parser's Factory and Predefinitions classes and never convert the strings to lower case.

The pull request is at 
https://bitbucket.org/TheOneRing/hg.fast-downward.org/pull-request/10

The evaluation can be found at 
http://winkde.org/~pvonreth/other/tmp/fd/issue456/. 
Make sure to select the second version of the experiments (v2). 

As always, the results are noisy, but overall coverage never decreases for any configuration.

Malte, can you have a look at the code, please? Should we branch off this issue to continue working on 
shared_pointers or wait until this part is merged?
msg4160 (view) Author: jendrik Date: 2015-04-13.16:58:34
Well, I haven't thought about the details, but in principle, we have to teach the parser how to 
handle smart pointers. I would think that this could be done similarly to the handling of raw 
pointers. Then we have to replace every occurence of "return new MyClass(opts);" by "return 
make_shared<MyClass>(opts);". Finally, we can probably remove the handling of raw pointers from the 
parser. 

I guess you're having trouble with the first step. Can you explain what exactly the problem is in a 
private mail maybe?
msg4159 (view) Author: pvonreth Date: 2015-04-13.16:05:02
I'm either doing it wrong or this would end up in huge amount of work.
Every usage of a pointer every call returning now a shared pointer etc would ha
have to be modified.

I think we need to discuss what exactly we want before I look further into it.
msg3335 (view) Author: malte Date: 2014-08-24.13:14:45
> Do we currently have any way to guarantee a certain evaluation order, i.e. can
> it happen that a combining evaluator is evaluated before its components?

No, this cannot happen. We have mechanisms in place to ensure that each
heuristic is only evaluated once. The evaluation order should be consistent
within one planner run but not across planner runs because the involved
heuristics are ordered by address.

Evaluators that aren't heuristics, on the other hand, are always reevaluated on
the fly. The assumption is that evaluators are cheap and heuristics are not.

Each combining evaluator is responsible for triggering the evaluation of its
components.

> If all heuristics are evaluated by the search, can a combining evaluator choose
> to not evaluate a heuristic. For example, it could evaluate a cheap heuristic
> for dead end detection and avoid evaluating a more expensive heuristic in dead
> ends like Blai's implementation of the Flow heuristic did.

No. You can choose not to evaluate a given evaluator, but once it is registered
as a heuristic it will be evaluated at the same time as all other heuristics.
The easiest way to implement such a lazy evaluation at the moment is to make the
heuristic itself aware that it doesn't need to be evaluated under certain
conditions and then do nothing when asked to evaluate itself.

(I think there are some interesting aspects in these points, but I don't think
it's related to this issue, unless you have a design in mind that would help
with both areas.)
msg3334 (view) Author: florian Date: 2014-08-23.22:46:39
I'm not sure if the following comments are relevant for this issue, but I think
they could influence the design, so here goes:

Do we currently have any way to guarantee a certain evaluation order, i.e. can
it happen that a combining evaluator is evaluated before its components?

If all heuristics are evaluated by the search, can a combining evaluator choose
to not evaluate a heuristic. For example, it could evaluate a cheap heuristic
for dead end detection and avoid evaluating a more expensive heuristic in dead
ends like Blai's implementation of the Flow heuristic did.

If the heuristics are evaluated by the combination heuristic, it might be
difficult to avoid evaluating them twice. issue77 might help with that.
msg3333 (view) Author: jendrik Date: 2014-08-23.01:09:05
Quoting our e-mail discussion:

Jendrik: "[...] We have no standard way of deciding who should be responsible for 
the deletion of objects passed as options like the MergeStrategy, the 
Heuristic or the open lists and their respective options."

Malte: "[...] It's not just about who should have the
responsibility, but rather that the objects currently have no way of
knowing if their objects are shared by anyone else (e.g. if a heuristic
is used in several combining evaluators, or if an evaluator is used in
several searches). The reason why we don't delete these objects at the
moment is because we don't have the information to know when they are no
longer needed.

I haven't looked at this from all angles, but from my initial
impression, I agree with Erez that this is a perfected use case for
shared_ptr. Simple reference counting (which shared_ptr essentially
does) should solve the lifetime tracking problem in our case, since our
option syntax doesn't allow cyclic dependencies.

We may have to be a bit careful with synergy objects, but we want to get
rid of them anyway."
History
Date User Action Args
2015-04-22 11:17:09jendriksetstatus: chatting -> resolved
2015-04-22 11:05:59jendriksetmessages: + msg4178
2015-04-21 16:29:48jendriksetmessages: + msg4177
2015-04-21 15:57:09jendriksetmessages: + msg4176
2015-04-21 15:28:57pvonrethsetmessages: + msg4175
2015-04-21 15:17:30maltesetmessages: + msg4174
2015-04-21 15:13:55maltesetmessages: + msg4173
2015-04-21 15:03:15pvonrethsetmessages: + msg4172
2015-04-21 14:07:40maltesetmessages: + msg4171
2015-04-21 11:24:27jendriksetmessages: + msg4170
2015-04-21 10:17:37pvonrethsettitle: use shared_ptr for objects passed as options -> use shared_ptr for AbstractTask
2015-04-13 16:58:34jendriksetmessages: + msg4160
2015-04-13 16:05:02pvonrethsetmessages: + msg4159
2015-04-13 14:27:11pvonrethsetassignedto: pvonreth
nosy: + pvonreth
2014-09-16 11:35:46silvansetnosy: + silvan
2014-08-24 13:14:45maltesetmessages: + msg3335
2014-08-23 22:46:39floriansetstatus: unread -> chatting
nosy: + florian
messages: + msg3334
2014-08-23 01:09:05jendrikcreate