Merged. There still is a lot of common code in SegmentedVector and
SegmentedArrayVector but this is handled in issue388.

The results for the bugfixed version are in:

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue387-issue387-compare.html
(WARNING: 50MB)


Most values stay the same. Memory consumption changes a bit (in particular in
sokoban-opt08-strips with configs that use merge&shrink). Is memory consumption
correctly reported for portfolios? A lot of the runs in both branches report a
value close to 2GB.

It looks like only a small part of the experiment is done yet, so I'd recommend
rerunning it.

There are only two deletes in that file, sorry for the stupid question. I fixed
it (also in this issue branch). The experiment is already running. Should I stop
and restart it?

Only the one in the changeset above. Where is the other one?

> I fixed this together with another bug were delete was used instead of delete[]
> (https://bitbucket.org/flogo/downward-issues-issue387/commits/fbe5219).

My local copy of state.cc contains two cases where delete[] should be used
instead of delete. Are both of them fixed?

The first experiment looks ok for all configs that do not use LM count:

http://ai.cs.unibas.ch/_tmp_files/pommeren/issue387-issue387-compare.html
(WARNING: 50MB)

For LM count, the issue was a wrong initialization which should only affect
configs that use PerStateInformation with non-build-in types. I fixed this
together with another bug were delete was used instead of delete[]
(https://bitbucket.org/flogo/downward-issues-issue387/commits/fbe5219).

The latter bug should have affected all configs in both revisions (i.e. also in
the default branch). It seems that the effect wasn't that big, otherwise we
would have caught it in the review of issue344.

I'll re-run the experiment with the fix in the issue branch. If there are any
significant differences to the default branch, we should merge only the bugfix
into the default branch and test again (and possibly also test issue344 again).
If there are no differences, we can just merge this issue branch.

Ok, I'll implement this before running tests.

I think resize() implemented via push_back() would be nice. The asymptotic
complexity would be the same as currently (because either way the added objects
need to be initialized), and I strongly doubt that the loss of efficiency would
be noticeable in our use cases, even if we added cases where we'd resize by
larger amounts.

I was going to say that the case of shrinking the vector could then be
implemented with pop_back, but we don't actually have a pop_back method
currently. Given this, I think it would be consistent to not allow shrinking.

I agree, it would be nice to reduce the complexity. One easy way to do this
would be to remove support for reducing the size. We currently only increase the
size.
In fact, we only increase the size by 1, so push_back would be sufficient, but
this is not obvious from looking at the code. This was the reason why we wanted
a resize method in the first place. We could also call push_back repeatedly
within resize. This would make the interface a bit nicer but otherwise keep the
code as it is currently.

Looks good at first glance. Before we review the details, can you run
experiments like the latest ones in issue344 to see the effect on run time/memory?

The only thing I dislike a bit is the complexity of the resize method. The class
becomes larger by ~40% lines of code with this change, and it's not code we're
going to exercise a lot. Ideally I'd prefer something geared more towards easy
understanding/less code, even if that makes it slightly less efficient. Once
we've established that speed and memory usage is fine, I might recommend some
simplifications of the resize() code.

I tried adding a resize method and switched from directly using arrays to
allocators. This is done in vector and allows to create a range of objects even
if the class has no default constructor.

Malte, could you have a look to see if this is what you had in mind:
https://bitbucket.org/flogo/downward-issues-issue387/pull-request/1/issue387/diff

Following up on our bitbucket discussion, I still don't understand the point of
having a default value in SegmentedVector. It is not a default-dict-type class
like PerStateInformation. It is really intended to replace a vector -- why
shouldn't we stick to the vector interface?

For resizing, I think we should add a resize method like the one vector has.

We split issue344 into more manageable chunks. The main issue branch will soon
be merged and this is a possible follow-up work. I nosied everyone from the
original issue, but feel free to erase yourself from nosy, if you are not
interested.

Default values are currently handled differently in PerStateInformation and
SegmentedVector. Resizing is done with repeated calls to push_back(). We should
have a look at how default values are handled in the vector class.