On Thu, Jan 14, 2010 at 11:28 AM, <[email protected]> wrote: > 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 >
To test that I artificially reduced the number of bits used to encode page index, but didn't find reasonable way to make a proper test out of it. I'd try to make a test following your idea or maybe something else. yours, anton.
-- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
