Title Make GlobalState independent from globals
Priority feature Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To florian Keywords
Optional summary
part of issue509

Created on 2016-08-12.19:12:54 by florian, last changed by florian.

part of issue509
msg5563 (view) Author: florian Date: 2016-08-16.17:29:50
msg5562 (view) Author: malte Date: 2016-08-16.00:36:57
Sounds good!
msg5561 (view) Author: florian Date: 2016-08-16.00:28:19
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):

If there are no objections, I'll update the code as you suggested in the comments and merge.
msg5560 (view) Author: malte Date: 2016-08-15.22:48:06
Looks good! I made some minor comments on Bitbucket.
msg5557 (view) Author: florian Date: 2016-08-15.14:30:07
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.
msg5555 (view) Author: malte Date: 2016-08-15.14:13:24
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?
msg5553 (view) Author: florian Date: 2016-08-15.13:55:13
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.
msg5552 (view) Author: malte Date: 2016-08-15.13:42:01
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
msg5551 (view) Author: jendrik Date: 2016-08-15.12:37:09
Fine with me.
msg5550 (view) Author: florian Date: 2016-08-15.12:33:17
Thanks. I adopted your comments. From my side, this is ready to merge now.
msg5548 (view) Author: jendrik Date: 2016-08-15.12:15:22
I left a few comments on bitbucket. Apart from these, the code and the experiment 
results look good to me.
msg5546 (view) Author: florian Date: 2016-08-15.11:52:49
I created a pull request on Bitbucket:

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.
msg5545 (view) Author: florian Date: 2016-08-12.19:12:54
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.
Date User Action Args
2016-08-16 17:29:50floriansetstatus: in-progress -> resolved
messages: + msg5563
2016-08-16 00:36:57maltesetmessages: + msg5562
2016-08-16 00:28:19floriansetmessages: + msg5561
2016-08-15 22:48:06maltesetmessages: + msg5560
2016-08-15 14:30:07floriansetmessages: + msg5557
2016-08-15 14:13:24maltesetmessages: + msg5555
2016-08-15 13:55:13floriansetmessages: + msg5553
2016-08-15 13:42:01maltesetmessages: + msg5552
2016-08-15 12:37:10jendriksetmessages: + msg5551
2016-08-15 12:33:17floriansetmessages: + msg5550
2016-08-15 12:15:22jendriksetmessages: + msg5548
2016-08-15 11:52:49floriansetmessages: + msg5546
2016-08-12 19:12:54floriancreate