Issue508

Title Get rid of TR1 dependencies
Priority bug Status resolved
Superseder Nosy List florian, gabi, jendrik, kcleung, malte, silvan
Assigned To jendrik Keywords
Optional summary

Created on 2015-01-25.18:35:27 by gabi, last changed by silvan.

Messages
msg4094 (view) Author: silvan Date: 2015-03-18.21:36:47
There was a bug introduced with this issue, in shrink_bucket_based.cc: replacing
slist::insert/splice with forward_list::insert/splice without adapting the
position at which one wants to insert/splice caused a problem with empty lists.
This is just an FYI in case anyone needs to replace any other slists by
forward_lists.
msg4080 (view) Author: malte Date: 2015-03-17.15:51:17
> We agreed to attribute this change in speed to "random" M&S fluctuations.

For those who weren't there: we reran the affected task on another machine and
came up with a similar difference of ~800 and ~1200 seconds -- but in the
opposite order. While this is not quite reassuring, it seems to be the common
kind of M&S madness that defies explanation and isn't worth worrying about while
Silvan is heavily refactoring the code anyway.
msg4079 (view) Author: jendrik Date: 2015-03-17.15:49:30
We agreed to attribute this change in speed to "random" M&S fluctuations. 

Merged and pushed.
msg4074 (view) Author: malte Date: 2015-03-17.14:38:06
I'm a bit shocked by the 50% slowdown in tidybot. Is this reproducible or a fluke?
msg4073 (view) Author: jendrik Date: 2015-03-17.14:21:17
The results are in: 
http://ai.cs.unibas.ch/_tmp_files/seipp/issue508-mas-issue508-base-issue508-v1-
compare.html

Judging by score_memory I'd say we could merge this. What do you think?
msg4072 (view) Author: jendrik Date: 2015-03-17.11:27:17
OK, I'll run the experiment.
msg4071 (view) Author: malte Date: 2015-03-17.11:16:12
When you do, please include the memory and score_memory attributes.
msg4070 (view) Author: florian Date: 2015-03-17.11:08:01
Looks good to me. Can someone (Jendrik?) run the benchmark on M&S?
msg4063 (view) Author: kcleung Date: 2015-03-16.23:36:41
Done, I have removed the LP includes, and it still compiles.  Please check my hg
branch again
msg4061 (view) Author: jendrik Date: 2015-03-16.11:23:50
Compiling works again. You can pull from master and remove the LP includes in 
your patch, kcleung.
msg4060 (view) Author: jendrik Date: 2015-03-16.07:42:32
Indeed the buildbot uses DOWNWARD_USE_LP=1 for all configurations. I will add a 
new configuration and then fix compilation with DOWNWARD_USE_LP=0.
msg4055 (view) Author: malte Date: 2015-03-14.19:55:06
Assigning this back to Gabi, so that it shows up when she displays her assigned
issues.

kcleung, please don't modify the "assigned to" field in the tracker. This should
only be used by those with push access to the master repository.
msg4054 (view) Author: malte Date: 2015-03-14.19:44:37
> As far as I know, the code should compile fine without COIN. The heuristics
> that require an LP solver link to
> http://www.fast-downward.org/LPBuildInstructions.
> Which part did not compile without the OSI libraries?

Florian, I think the recently merged issue443 broke the build when using
DOWNWARD_USE_LP=0. With the current version of default, I cannot compile the
planner with DOWNWARD_USE_LP=0 on either my machine or maia (in both cases using
some variant of gcc 4.8.2). If I go to the last version of default before
merging in issue443, it works. The error message looks like we're using an
incomplete type together with unique_ptr, which I don't think is supported
because the destructor of the pointer needs the full type definition. Can you
look into this?

Steps to reproduce:

cd src
hg up issue443-v4-base  # last version before merge
./build_all distclean
./build_all -j4

(Should produce an error when compiling certain source files that depend on the
LP code, such as lp_solver.cc.)

I wonder why the buildbot didn't catch this. Do we only test with
DOWNWARD_USE_LP=1? I think it would be useful to have at least one configuration
with either setting of DOWNWARD_USE_LP because this is not the first time that
one of the two variants breaks and we didn't notice immediately.
msg4053 (view) Author: kcleung Date: 2015-03-13.20:19:08
> As far as I know, the code should compile fine without COIN. 
> The heuristics that require an LP solver link to 
> http://www.fast-downward.org/LPBuildInstructions.
> Which part did not compile without the OSI libraries?

search/lp_internals.h and search/lp_solver.h

If I don't have the OSI libraries and add #include <OsiSolverInterface.hpp>

then the compiler will complain of forward declaration of incomplete type

Without the changes I made, I could not get it compiled on my OS X 10.10

> Also, I think CoinMP is a project containing the OSI (open solver interface),
> CLP (LP solver) and some other additional libraries. We only require the OSI
> libraries and an LP solver and we recommend Cplex over CLP.
msg4051 (view) Author: florian Date: 2015-03-13.07:10:23
> 1) Florian to have a look at the new LP #includes. Florian, do they look good to
> you?

We have forward declarations to these classes, so I don't think we need the
include. Is there a problem with forward declarations on OS X?

> One more thing: in the documentation, we need to make it clear that the search 
> code depends on the Coin MP library:

As far as I know, the code should compile fine without COIN. The heuristics that
require an LP solver link to http://www.fast-downward.org/LPBuildInstructions.
Which part did not compile without the OSI libraries?

Also, I think CoinMP is a project containing the OSI (open solver interface),
CLP (LP solver) and some other additional libraries. We only require the OSI
libraries and an LP solver and we recommend Cplex over CLP.
msg4048 (view) Author: kcleung Date: 2015-03-12.23:26:27
One more thing: in the documentation, we need to make it clear that the search 
code depends on the Coin MP library:

https://projects.coin-or.org/CoinMP
msg4047 (view) Author: malte Date: 2015-03-12.22:05:30
Thanks! The changes look good to me. Before we merge this, I'd like

1) Florian to have a look at the new LP #includes. Florian, do they look good to
you?

2) Someone to benchmark the changes from slist to forward_list. I assume these
are essentially identical implementations of singly-linked lists, but there have
been dangerous performance traps in this code in the past. An experiment with
our basic merge-and-shrink configurations should be sufficient. Any volunteers?

> One more thing:  Does the binary really have to be 32-bit?
> Currently the use of -m32 prevents compilation on ubuntu 15.04 amd64

Feel free to use the option to compile with 64 bit instead (e.g. with "export
DOWNWARD_BITWIDTH=64" when using the planner for purposes other than performance
comparison to other planners. But please don't do this in experiments for
published papers that compare to other planners, because we expect people to run
the planner with full optimizations in such cases. A 64-bit compile
unfortunately still had a significant negative performance impact last time we
tried it (mainly on memory usage). Once a 64-bit compile is competitive with a
32-bit compile, we'd be happy to get rid of the -m32 flag -- the 64-bit
performance doesn't even have to be equally good to 32-bit performance, it just
should be close enough.

Can you describe the issue that prevents compiling with -m32 on Ubuntu 15.04? Is
the g++-multilib package unavailable or broken on 15.04, or do you receive
linking errors? Generally we try to support most common platforms (like Ubuntu)
even when this means working around platform bugs. (However, I'd rather not have
us spent too much time on this while Ubuntu 15.04 isn't released, because
chasing bugs on unreleased OSes often means trying to hit a moving target. The
last month before an OS version is released often has lots of changes to the
platform.)

Note: whenever you change the bitwidth, you have to run "make distclean" before
recompiling.
msg4046 (view) Author: kcleung Date: 2015-03-12.21:49:36
I have removed all TR1 and ext/slist dependencies, and now it compiles 
perfectly on OS X 10.10.  

Please pull my changes at:

https://bitbucket.org/kcleung/hg.fast-downward.org/branch/fix-libcpp

One more thing:  Does the binary really have to be 32-bit?

Currently the use of -m32 prevents compilation on ubuntu 15.04 amd64
msg3999 (view) Author: malte Date: 2015-01-25.23:26:22
Yes, this should be fixable by just including the #includes and namespaces used.
msg3998 (view) Author: gabi Date: 2015-01-25.18:35:27
There appears to a be a problem with the newest OS X C++ compiler, which doesn't
support using C++-11 and TR1 in parallel.

Since we now require C++11 anyway, we could just remove all TR1 dependencies. I
don't think that we use anything that did not become part of the standard.
History
Date User Action Args
2015-03-18 21:36:47silvansetnosy: + silvan
messages: + msg4094
2015-03-17 15:52:19maltesetstatus: reviewing -> resolved
2015-03-17 15:51:17maltesetmessages: + msg4080
2015-03-17 15:49:31jendriksetmessages: + msg4079
2015-03-17 14:38:06maltesetmessages: + msg4074
2015-03-17 14:21:17jendriksetassignedto: gabi -> jendrik
messages: + msg4073
2015-03-17 11:27:17jendriksetmessages: + msg4072
2015-03-17 11:16:12maltesetmessages: + msg4071
2015-03-17 11:08:01floriansetmessages: + msg4070
2015-03-16 23:36:41kcleungsetmessages: + msg4063
2015-03-16 11:23:50jendriksetmessages: + msg4061
2015-03-16 07:42:32jendriksetmessages: + msg4060
2015-03-14 19:55:06maltesetassignedto: kcleung -> gabi
messages: + msg4055
2015-03-14 19:44:37maltesetmessages: + msg4054
2015-03-13 20:19:08kcleungsetmessages: + msg4053
2015-03-13 07:10:23floriansetmessages: + msg4051
2015-03-12 23:26:27kcleungsetmessages: + msg4048
2015-03-12 22:05:30maltesetmessages: + msg4047
2015-03-12 21:49:37kcleungsetstatus: chatting -> reviewing
assignedto: gabi -> kcleung
messages: + msg4046
nosy: + kcleung
2015-02-02 15:51:15floriansetnosy: + florian
2015-01-28 23:23:22gabisetassignedto: gabi
2015-01-25 23:26:22maltesetstatus: unread -> chatting
nosy: + malte
messages: + msg3999
2015-01-25 19:20:58jendriksetnosy: + jendrik
2015-01-25 18:35:27gabicreate