part of issue509

Merged.

Sounds good!

Thank you! The results for the latest version also look good. Actually, they look much better 
than I expected. I expected no change at all, but there seems to be a clear positive trend in 
runtime (not a big effect, but clearly visible in the plot):

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue665-v2-issue665-base-issue665-v2-compare.html
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue665-v2-total_time-astar-blind.png

If there are no objections, I'll update the code as you suggested in the comments and merge.

Looks good! I made some minor comments on Bitbucket.

The state registry currently doesn't know/store the task, but this would a lot of 
sense in the long run. We planed to switch the registry to the task interface 
later in issue509. At least then it would probably need to know the task.

For this issue, I suggest to add a task argument to the constructor of the 
registry, store it and add a public getter. The task would not be used for 
anything else right now, because we still have to finish some other changes 
first, but we could use it in the dump methods.

We could also use this way to access the number of variables, but then we would 
have to construct a task proxy every time.

I see, that clears up some of the comments.

For dump_pddl and dump_fdr, this means that the second parameter must match the
task to which this state belongs. This is still a code smell: there is only one
correct object that can be passed in here, and the state should be able to find
this out by itself. Can't the state find out what the underlying task is by
asking the state registry?

Thanks for the comments, Malte.

I think the function argument is not redundant. Currently, all state registries use the 
global task, but a user could create their own state registry by creating IntPacker, 
AxiomEvaluator and initial state objects from a different task. "GlobalStates" created 
from such a registry would then belong to a different task. The name GlobalState is 
confusing in this case; judging from the functionality the class should be called 
"PackedRegisteredState". I didn't want to rename the class in this issue, though.

I also thought about getting the number of variables from the registry but I thought the 
overhead of the function call would be critical. But I guess with inlining it could be 
fine. I'll try this and run a new experiment.

I made a few comments on Bitbucket.

I think this currently goes into the wrong direction in terms of code quality.
It trades one code smell (use of global variables) with two more severe ones
(instance attributes that hold the same value in all instances of a class;
function parameters that must be passed the same value in all function
invocations). I understand the motivation of getting rid of the globals, but
this doesn't really conceptually remove them, just sweep them under the rug.

It seems that num_variables must always equal the number of variables of the
state registry that the state belongs to, so one possible way to proceed is to
remove GlobalState::num_variables and ask for the number of variables of the
state registry where this is needed instead. (This may require adding a new
public method to query this if it cannot currently be requested, and I'm not
sure this is great, but I think it would be a less severe issue than the current
patch.)

Fine with me.

Thanks. I adopted your comments. From my side, this is ready to merge now.

I left a few comments on bitbucket. Apart from these, the code and the experiment 
results look good to me.

I created a pull request on Bitbucket:

https://bitbucket.org/FlorianPommerening/downward-issues/pull-requests/23/issue665/diff

Experiments look ok. Coverage drops by one task, but this task was solved just within the time limit (5 min) by the baseline. Looking at the distribution of total time, it looks very evenly 
distributed between increases and decreases, so I would attribute this to noise.

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue665-v1-total_time-astar-blind.png
http://ai.cs.unibas.ch/_tmp_files/pommeren/issue665-v1-issue665-base-issue665-v1-compare.html

GlobalState currently depends on global variables in three ways:
1. Indirectly, through the state registry:
   The only way to create a GlobalState is through a state registry and
   all our state registries currently use the global root task.
2. The methods GlobalState::dump* use globals to access the names and
   numbers of variables and facts.
3. GlobalState::get_values uses g_variable_domain to access the number
   of variables.

We suggest to get rid of 2. and 3. by removing GlobalState::dump* and storing the 
number of variables in GlobalState.

Any use of GlobalState::dump* can be replaced by transforming the state into a 
State object of the appropriate task and then using State::dump*.

Storing the state size in GlobalState can be done if we also store it in 
StateRegistry. Memory shouldn't be a big problem as we never have a large number of 
GlobalState objects in memory at the same time.