Fix map compact implementation....

Fix map compact implementation.

Always invoke HeapObjectIterator::has_next() before invoking HeapObjectIterator::next().
This is necessary as ::has_next() has an important side-effect of going to the next
page when current page is exhausted.

And to find if pointers are encodable use more precise data---top of map space, not a number
of pages, as pages might stay in map space due to chunking.


Committed: http://code.google.com/p/v8/source/detail?r=3672

[antonm, 2010-01-20 16:48:23]

Søren,

I was surprised to find necessity to invoke has_next as otherwise iterator goes
wild.

For now I fixed it by stupid it.has_next() invocations.  More principal approach
could be to blend has_next and next into one method (e.g. making next return 0
when space is exhausted.)  If you think it's a reasonable idea, I'd willingly
implement it, but I'd prefer to do that in another CL to ease rolling back if
need would be.

[antonm, 2010-01-20 16:49:36]

(sending again as I made a type in the email.)

Søren,

I was surprised to find necessity to invoke has_next as otherwise iterator goes
wild.

For now I fixed it by stupid it.has_next() invocations.  More principal approach
could be to blend has_next and next into one method (e.g. making next return 0
when space is exhausted.)  If you think it's a reasonable idea, I'd willingly
implement it, but I'd prefer to do that in another CL to ease rolling back if
need would be.

[Søren Thygesen Gjesse, 2010-01-21 07:57:41]

LGTM

I think you should raise a bug on the iterator next()/has_next() semantics to
track this issue. Maybe add a reference from the bug to this change.

http://codereview.chromium.org/552066/diff/1/5
File src/mark-compact.cc (right):

http://codereview.chromium.org/552066/diff/1/5#newcode1294
src/mark-compact.cc:1294: it.has_next();
Please add a comment here.

http://codereview.chromium.org/552066/diff/1/3
File src/spaces.h (right):

http://codereview.chromium.org/552066/diff/1/3#newcode1803
src/spaces.h:1803: it.has_next();
Please add a comment here.

http://codereview.chromium.org/552066/diff/1/3#newcode1807
src/spaces.h:1807: it.has_next();
And here.

[antonm, 2010-01-21 14:25:58]

Thanks a lot for review, Søren.

http://code.google.com/p/v8/issues/detail?id=586 filed and referenced in changed
files.

yours,
anton.

On 2010/01/21 07:57:41, Søren Gjesse wrote:
> LGTM
> 
> I think you should raise a bug on the iterator next()/has_next() semantics to
> track this issue. Maybe add a reference from the bug to this change.
> 
> http://codereview.chromium.org/552066/diff/1/5
> File src/mark-compact.cc (right):
> 
> http://codereview.chromium.org/552066/diff/1/5#newcode1294
> src/mark-compact.cc:1294: it.has_next();
> Please add a comment here.
> 
> http://codereview.chromium.org/552066/diff/1/3
> File src/spaces.h (right):
> 
> http://codereview.chromium.org/552066/diff/1/3#newcode1803
> src/spaces.h:1803: it.has_next();
> Please add a comment here.
> 
> http://codereview.chromium.org/552066/diff/1/3#newcode1807
> src/spaces.h:1807: it.has_next();
> And here.