On 2010/01/13 19:15:36, antonm wrote:
Thanks a lot for review, Søren, committing.
http://codereview.chromium.org/509035/diff/5006/5008
File src/mark-compact.cc (right):
http://codereview.chromium.org/509035/diff/5006/5008#newcode1412
src/mark-compact.cc:1412: ASSERT(Heap::map_space()->Contains(new_map));
On 2010/01/13 08:55:30, Søren Gjesse wrote:
> On 2010/01/12 16:53:36, antonm wrote:
> > On 2010/01/12 08:07:14, Søren Gjesse wrote:
> > > Does this ASSERT check against the resulting size of map space after
> > compaction?
> >
> > Very good question. I don't think so as the space shouldn't have been
shrunk
> > yet.
> >
> > I've added the check, but it feels somewhat ugly to me (esp. this
> recalculation
> > of # of live maps). Maybe we'd better add this check (if it's not
present
> right
> > now) into post GC verification code and remove it from here not to
confuse
> > readers?
> >
>
> You are right it it both ugly and slow. Your suggestion to add some map
space
> verification sounds much better,
Ok. I double checked and it looks like all the cctest are ran with
verify_heap
flag on, so we should get it for free. Just removing.
http://codereview.chromium.org/509035/diff/5006/5008#newcode1483
src/mark-compact.cc:1483: MapCompact map_compact(from_space);
On 2010/01/13 08:55:30, Søren Gjesse wrote:
> On 2010/01/12 16:53:36, antonm wrote:
> > On 2010/01/12 08:07:14, Søren Gjesse wrote:
> > > How about passing in Heap::map_space() explicitly as the to space
here?
> >
> > Sorry, don't quite understand you, could you elaborate?
> >
> > In case it's due to bad naming, just dropped the var :)
>
> My suggenstion was to add another parameter indicating the from space,
but
you
> can ignore that. Thinking about it again how about dropping the current
> parameter? It's up to you.
Good idea, done. I used to depend on it in the body before, but not now.
Note:
couldn't drop it completely as it depends on live map count.
I talked with Kasper about adding a test for this. How taking the JavaScript
code from test/mjsunit/regress/regress-524.js and allocate even more to
exceed
the map compaction limit and then freeing up data to make map compaction
run. It
will probably require a cctest.
http://codereview.chromium.org/509035
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
