About the tests: Instead of changing the old tests to
FLAG_retain_maps_for_n_gc = 0; wouldn't it make more sense to change the
test
semantics?
Most of them test for leaks, so we would need to do |
FLAG_retain_maps_for_n_gc|
number of GCs instead of one GC. TEST(MapRetaining) checks that there are no
leaks introduced by this CL, and I thought that justifies usage of
FLAG_retain_maps_for_n_gc = 0.
I can change the tests to run |FLAG_retain_maps_for_n_gc| GC if you think
that
would be better (I don't have preference).
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc#newcode2127
src/heap/mark-compact.cc:2127: if (reduce_memory_footprint_ ||
abort_incremental_marking_ ||
On 2014/12/16 17:15:35, Hannes Payer wrote:
Why do you bail out for abort_incremental_marking_?
The incremental marking is aborted for GC requested explicitly by tests
and dev-tools. In that cases we want to be precise. I added a comment in
the code.
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc#newcode2134
src/heap/mark-compact.cc:2134: // down to Map::kRetainingCounterStart.
This range can be narrowed
On 2014/12/16 16:16:32, Erik Corry wrote:
Comment looks wrong.
Done.
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc#newcode2159
src/heap/mark-compact.cc:2159: ProcessMarkingDeque();
On 2014/12/16 16:52:48, Erik Corry wrote:
Can't we just process the marking deque at the end, instead of inside
the loop?
By doing it here you may mark maps live that you haven't got to yet,
and they
won't age, even though they are only reachable through maps that are
ageing.
Done.
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc#newcode2161
src/heap/mark-compact.cc:2161: if (counter <
Map::kRetainingCounterStart) {
On 2014/12/16 17:15:35, Hannes Payer wrote:
else if
Done.
https://codereview.chromium.org/794583003/diff/60001/src/heap/mark-compact.cc#newcode2269
src/heap/mark-compact.cc:2269: RetainMaps(&root_visitor);
On 2014/12/16 17:15:35, Hannes Payer wrote:
On 2014/12/16 16:35:23, ulan wrote:
> On 2014/12/16 16:16:33, Erik Corry wrote:
> > This should be after the ephemeral stuff. Maps that are kept
alive by
> > ephemerally-alive objects should not age.
> >
> > But even better would be to put it inside ClearNonLiveReferences.
That
already
> > iterates over the map space. It is better for the cache not to
iterate
> multiple
> > times.
>
> It should not be after ephemeral marking, because a resurrected map
could mark
> the key of an ephemeron, which would require marking the value of
the
ephemeron.
>
> The most precise would be to put RetainMaps inside the loop in
> ProcessEphemeralMarking, but that would be prohibitively expensive.
So I had
to
> use imprecise (some live maps will age) but faster approach.
Please write a comment about that somewhere.
Done.
https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/794583003/diff/60001/test/cctest/test-heap.cc#newcode4955
test/cctest/test-heap.cc:4955: TEST(MapRetaining) {
On 2014/12/16 17:15:35, Hannes Payer wrote:
Please also test the flag for n == 0, n in between start and end, and
n outside
start and end.
Done.
https://codereview.chromium.org/794583003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
