From ba70a7a3e189bc11da38c2cacf4900794ac53214 Mon Sep 17 00:00:00 2001 From: hpayer Date: Thu, 9 Apr 2015 01:55:37 -0700 Subject: [PATCH] MarkBit cleanup: They have to be accessed through Marking accessors. Avoid arbitrary abuse of mark bits and make marking explicit. Added DCHECKs to mark bit transitions to check source state. BUG= Review URL: https://codereview.chromium.org/1040443002 Cr-Commit-Position: refs/heads/master@{#27689} --- src/heap/heap.cc | 6 +- src/heap/incremental-marking.cc | 3 +- src/heap/mark-compact-inl.h | 11 ++-- src/heap/mark-compact.cc | 111 ++++++++++++++++++-------------- src/heap/mark-compact.h | 45 ++++++++++++- src/heap/objects-visiting-inl.h | 4 +- src/heap/spaces.cc | 6 +- src/heap/spaces.h | 17 ++--- src/heap/store-buffer.cc | 3 +- 9 files changed, 129 insertions(+), 77 deletions(-) diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 1fe546386b..a50db96b8b 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -5771,7 +5771,7 @@ class UnreachableObjectsFilter : public HeapObjectsFilter { bool SkipObject(HeapObject* object) { MarkBit mark_bit = Marking::MarkBitFrom(object); - return !mark_bit.Get(); + return Marking::IsWhite(mark_bit); } private: @@ -5784,8 +5784,8 @@ class UnreachableObjectsFilter : public HeapObjectsFilter { if (!(*p)->IsHeapObject()) continue; HeapObject* obj = HeapObject::cast(*p); MarkBit mark_bit = Marking::MarkBitFrom(obj); - if (!mark_bit.Get()) { - mark_bit.Set(); + if (Marking::IsWhite(mark_bit)) { + Marking::WhiteToBlack(mark_bit); marking_stack_.Add(obj); } } diff --git a/src/heap/incremental-marking.cc b/src/heap/incremental-marking.cc index e01c50b849..dfcd68a3d3 100644 --- a/src/heap/incremental-marking.cc +++ b/src/heap/incremental-marking.cc @@ -141,7 +141,6 @@ static inline void MarkBlackOrKeepBlack(HeapObject* heap_object, if (Marking::IsBlack(mark_bit)) return; Marking::MarkBlack(mark_bit); MemoryChunk::IncrementLiveBytesFromGC(heap_object->address(), size); - DCHECK(Marking::IsBlack(mark_bit)); } @@ -251,7 +250,7 @@ class IncrementalMarkingMarkingVisitor HeapObject* heap_object = HeapObject::cast(obj); MarkBit mark_bit = Marking::MarkBitFrom(heap_object); if (Marking::IsWhite(mark_bit)) { - mark_bit.Set(); + Marking::MarkBlack(mark_bit); MemoryChunk::IncrementLiveBytesFromGC(heap_object->address(), heap_object->Size()); return true; diff --git a/src/heap/mark-compact-inl.h b/src/heap/mark-compact-inl.h index 757b6c347a..fe32df0148 100644 --- a/src/heap/mark-compact-inl.h +++ b/src/heap/mark-compact-inl.h @@ -28,10 +28,9 @@ void MarkCompactCollector::SetFlags(int flags) { void MarkCompactCollector::MarkObject(HeapObject* obj, MarkBit mark_bit) { DCHECK(Marking::MarkBitFrom(obj) == mark_bit); - if (!mark_bit.Get()) { - mark_bit.Set(); + if (Marking::IsWhite(mark_bit)) { + Marking::WhiteToBlack(mark_bit); MemoryChunk::IncrementLiveBytesFromGC(obj->address(), obj->Size()); - DCHECK(IsMarked(obj)); DCHECK(obj->GetIsolate()->heap()->Contains(obj)); marking_deque_.PushBlack(obj); } @@ -39,9 +38,9 @@ void MarkCompactCollector::MarkObject(HeapObject* obj, MarkBit mark_bit) { void MarkCompactCollector::SetMark(HeapObject* obj, MarkBit mark_bit) { - DCHECK(!mark_bit.Get()); + DCHECK(Marking::IsWhite(mark_bit)); DCHECK(Marking::MarkBitFrom(obj) == mark_bit); - mark_bit.Set(); + Marking::WhiteToBlack(mark_bit); MemoryChunk::IncrementLiveBytesFromGC(obj->address(), obj->Size()); } @@ -49,7 +48,7 @@ void MarkCompactCollector::SetMark(HeapObject* obj, MarkBit mark_bit) { bool MarkCompactCollector::IsMarked(Object* obj) { DCHECK(obj->IsHeapObject()); HeapObject* heap_object = HeapObject::cast(obj); - return Marking::MarkBitFrom(heap_object).Get(); + return Marking::IsBlackOrGrey(Marking::MarkBitFrom(heap_object)); } diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc index 931206ff43..1bbe0e11cc 100644 --- a/src/heap/mark-compact.cc +++ b/src/heap/mark-compact.cc @@ -474,9 +474,7 @@ void MarkCompactCollector::ClearMarkbits() { LargeObjectIterator it(heap_->lo_space()); for (HeapObject* obj = it.Next(); obj != NULL; obj = it.Next()) { - MarkBit mark_bit = Marking::MarkBitFrom(obj); - mark_bit.Clear(); - mark_bit.Next().Clear(); + Marking::MarkWhite(Marking::MarkBitFrom(obj)); Page::FromAddress(obj->address())->ResetProgressBar(); Page::FromAddress(obj->address())->ResetLiveBytes(); } @@ -563,6 +561,34 @@ void MarkCompactCollector::RefillFreeList(PagedSpace* space) { } +void Marking::SetAllMarkBitsInRange(MarkBit start, MarkBit end) { + MarkBit::CellType* start_cell = start.cell(); + MarkBit::CellType* end_cell = end.cell(); + MarkBit::CellType start_mask = ~(start.mask() - 1); + MarkBit::CellType end_mask = (end.mask() << 1) - 1; + + if (start_cell == end_cell) { + *start_cell |= start_mask & end_mask; + } else { + *start_cell |= start_mask; + for (MarkBit::CellType* cell = start_cell + 1; cell < end_cell; cell++) { + *cell = ~0; + } + *end_cell |= end_mask; + } +} + + +void Marking::ClearAllMarkBitsOfCellsContainedInRange(MarkBit start, + MarkBit end) { + MarkBit::CellType* start_cell = start.cell(); + MarkBit::CellType* end_cell = end.cell(); + for (MarkBit::CellType* cell = start_cell; cell <= end_cell; cell++) { + *cell = 0; + } +} + + void Marking::TransferMark(Address old_start, Address new_start) { // This is only used when resizing an object. DCHECK(MemoryChunk::FromAddress(old_start) == @@ -583,14 +609,11 @@ void Marking::TransferMark(Address old_start, Address new_start) { #endif if (Marking::IsBlack(old_mark_bit)) { - old_mark_bit.Clear(); - DCHECK(IsWhite(old_mark_bit)); + Marking::BlackToWhite(old_mark_bit); Marking::MarkBlack(new_mark_bit); return; } else if (Marking::IsGrey(old_mark_bit)) { - old_mark_bit.Clear(); - old_mark_bit.Next().Clear(); - DCHECK(IsWhite(old_mark_bit)); + Marking::GreyToWhite(old_mark_bit); heap_->incremental_marking()->WhiteToGreyAndPush( HeapObject::FromAddress(new_start), new_mark_bit); heap_->incremental_marking()->RestartIfNotMarking(); @@ -970,7 +993,7 @@ void CodeFlusher::ProcessJSFunctionCandidates() { Code* code = shared->code(); MarkBit code_mark = Marking::MarkBitFrom(code); - if (!code_mark.Get()) { + if (Marking::IsWhite(code_mark)) { if (FLAG_trace_code_flushing && shared->is_compiled()) { PrintF("[code-flushing clears: "); shared->ShortPrint(); @@ -979,6 +1002,7 @@ void CodeFlusher::ProcessJSFunctionCandidates() { shared->set_code(lazy_compile); candidate->set_code(lazy_compile); } else { + DCHECK(Marking::IsBlack(code_mark)); candidate->set_code(code); } @@ -1012,7 +1036,7 @@ void CodeFlusher::ProcessSharedFunctionInfoCandidates() { Code* code = candidate->code(); MarkBit code_mark = Marking::MarkBitFrom(code); - if (!code_mark.Get()) { + if (Marking::IsWhite(code_mark)) { if (FLAG_trace_code_flushing && candidate->is_compiled()) { PrintF("[code-flushing clears: "); candidate->ShortPrint(); @@ -1050,8 +1074,8 @@ void CodeFlusher::ProcessOptimizedCodeMaps() { i += SharedFunctionInfo::kEntryLength) { Code* code = Code::cast(code_map->get(i + SharedFunctionInfo::kCachedCodeOffset)); - if (!Marking::MarkBitFrom(code).Get()) continue; - + if (Marking::IsWhite(Marking::MarkBitFrom(code))) continue; + DCHECK(Marking::IsBlack(Marking::MarkBitFrom(code))); // Move every slot in the entry. for (int j = 0; j < SharedFunctionInfo::kEntryLength; j++) { int dst_index = new_length++; @@ -1327,7 +1351,7 @@ class MarkCompactMarkingVisitor // Returns true if object needed marking and false otherwise. INLINE(static bool MarkObjectWithoutPush(Heap* heap, HeapObject* object)) { MarkBit mark_bit = Marking::MarkBitFrom(object); - if (!mark_bit.Get()) { + if (Marking::IsWhite(mark_bit)) { heap->mark_compact_collector()->SetMark(object, mark_bit); return true; } @@ -1378,7 +1402,7 @@ class MarkCompactMarkingVisitor collector->RecordSlot(start, p, o); HeapObject* obj = HeapObject::cast(o); MarkBit mark = Marking::MarkBitFrom(obj); - if (mark.Get()) continue; + if (Marking::IsBlackOrGrey(mark)) continue; VisitUnmarkedObject(collector, obj); } return true; @@ -1723,7 +1747,7 @@ class RootMarkingVisitor : public ObjectVisitor { // Replace flat cons strings in place. HeapObject* object = ShortCircuitConsString(p); MarkBit mark_bit = Marking::MarkBitFrom(object); - if (mark_bit.Get()) return; + if (Marking::IsBlackOrGrey(mark_bit)) return; Map* map = object->map(); // Mark the object. @@ -1754,7 +1778,7 @@ class StringTableCleaner : public ObjectVisitor { for (Object** p = start; p < end; p++) { Object* o = *p; if (o->IsHeapObject() && - !Marking::MarkBitFrom(HeapObject::cast(o)).Get()) { + Marking::IsWhite(Marking::MarkBitFrom(HeapObject::cast(o)))) { if (finalize_external_strings) { DCHECK(o->IsExternalString()); heap_->FinalizeExternalString(String::cast(*p)); @@ -1787,7 +1811,8 @@ typedef StringTableCleaner ExternalStringTableCleaner; class MarkCompactWeakObjectRetainer : public WeakObjectRetainer { public: virtual Object* RetainAs(Object* object) { - if (Marking::MarkBitFrom(HeapObject::cast(object)).Get()) { + if (Marking::IsBlackOrGrey( + Marking::MarkBitFrom(HeapObject::cast(object)))) { return object; } else if (object->IsAllocationSite() && !(AllocationSite::cast(object)->IsZombie())) { @@ -1901,14 +1926,15 @@ int MarkCompactCollector::DiscoverAndEvacuateBlackObjectsOnPage( offset += trailing_zeros; Address address = cell_base + offset * kPointerSize; HeapObject* object = HeapObject::FromAddress(address); + DCHECK(Marking::IsBlack(Marking::MarkBitFrom(object))); int size = object->Size(); survivors_size += size; Heap::UpdateAllocationSiteFeedback(object, Heap::RECORD_SCRATCHPAD_SLOT); - offset++; - current_cell >>= 1; + offset += 2; + current_cell >>= 2; // TODO(hpayer): Refactor EvacuateObject and call this function instead. if (heap()->ShouldBePromoted(object->address(), size) && @@ -1966,7 +1992,7 @@ bool MarkCompactCollector::IsUnmarkedHeapObject(Object** p) { if (!o->IsHeapObject()) return false; HeapObject* heap_object = HeapObject::cast(o); MarkBit mark = Marking::MarkBitFrom(heap_object); - return !mark.Get(); + return Marking::IsWhite(mark); } @@ -1976,7 +2002,7 @@ bool MarkCompactCollector::IsUnmarkedHeapObjectWithHeap(Heap* heap, DCHECK(o->IsHeapObject()); HeapObject* heap_object = HeapObject::cast(o); MarkBit mark = Marking::MarkBitFrom(heap_object); - return !mark.Get(); + return Marking::IsWhite(mark); } @@ -1984,7 +2010,7 @@ void MarkCompactCollector::MarkStringTable(RootMarkingVisitor* visitor) { StringTable* string_table = heap()->string_table(); // Mark the string table itself. MarkBit string_table_mark = Marking::MarkBitFrom(string_table); - if (!string_table_mark.Get()) { + if (Marking::IsWhite(string_table_mark)) { // String table could have already been marked by visiting the handles list. SetMark(string_table, string_table_mark); } @@ -2174,21 +2200,21 @@ void MarkCompactCollector::RetainMaps() { int new_age; Map* map = Map::cast(cell->value()); MarkBit map_mark = Marking::MarkBitFrom(map); - if (!map_mark.Get()) { + if (Marking::IsWhite(map_mark)) { if (age == 0) { // The map has aged. Do not retain this map. continue; } Object* constructor = map->GetConstructor(); - if (!constructor->IsHeapObject() || - !Marking::MarkBitFrom(HeapObject::cast(constructor)).Get()) { + if (!constructor->IsHeapObject() || Marking::IsWhite(Marking::MarkBitFrom( + HeapObject::cast(constructor)))) { // The constructor is dead, no new objects with this map can // be created. Do not retain this map. continue; } Object* prototype = map->prototype(); if (prototype->IsHeapObject() && - !Marking::MarkBitFrom(HeapObject::cast(prototype)).Get()) { + Marking::IsWhite(Marking::MarkBitFrom(HeapObject::cast(prototype)))) { // The prototype is not marked, age the map. new_age = age - 1; } else { @@ -2402,7 +2428,7 @@ void MarkCompactCollector::ClearNonLiveReferences() { ClearNonLivePrototypeTransitions(map); ClearNonLiveMapTransitions(map, map_mark); - if (!map_mark.Get()) { + if (Marking::IsWhite(map_mark)) { have_code_to_deoptimize_ |= map->dependent_code()->MarkCodeForDeoptimization( isolate(), DependentCode::kWeakCodeGroup); @@ -2470,8 +2496,8 @@ void MarkCompactCollector::ClearNonLiveMapTransitions(Map* map, // Follow back pointer, check whether we are dealing with a map transition // from a live map to a dead path and in case clear transitions of parent. - bool current_is_alive = map_mark.Get(); - bool parent_is_alive = Marking::MarkBitFrom(parent).Get(); + bool current_is_alive = Marking::IsBlackOrGrey(map_mark); + bool parent_is_alive = Marking::IsBlackOrGrey(Marking::MarkBitFrom(parent)); if (!current_is_alive && parent_is_alive) { ClearMapTransitions(parent, map); } @@ -2481,7 +2507,7 @@ void MarkCompactCollector::ClearNonLiveMapTransitions(Map* map, // Clear a possible back pointer in case the transition leads to a dead map. // Return true in case a back pointer has been cleared and false otherwise. bool MarkCompactCollector::ClearMapBackPointer(Map* target) { - if (Marking::MarkBitFrom(target).Get()) return false; + if (Marking::IsBlackOrGrey(Marking::MarkBitFrom(target))) return false; target->SetBackPointer(heap_->undefined_value(), SKIP_WRITE_BARRIER); return true; } @@ -3550,31 +3576,18 @@ static bool SetMarkBitsUnderInvalidatedCode(Code* code, bool value) { uint32_t end_index = MemoryChunk::FastAddressToMarkbitIndex(code_end - kPointerSize); + // TODO(hpayer): Filter out invalidated code in + // ClearInvalidSlotsBufferEntries. Bitmap* b = p->markbits(); MarkBit start_mark_bit = b->MarkBitFromIndex(start_index); MarkBit end_mark_bit = b->MarkBitFromIndex(end_index); - MarkBit::CellType* start_cell = start_mark_bit.cell(); - MarkBit::CellType* end_cell = end_mark_bit.cell(); - if (value) { - MarkBit::CellType start_mask = ~(start_mark_bit.mask() - 1); - MarkBit::CellType end_mask = (end_mark_bit.mask() << 1) - 1; - - if (start_cell == end_cell) { - *start_cell |= start_mask & end_mask; - } else { - *start_cell |= start_mask; - for (MarkBit::CellType* cell = start_cell + 1; cell < end_cell; cell++) { - *cell = ~0; - } - *end_cell |= end_mask; - } + Marking::SetAllMarkBitsInRange(start_mark_bit, end_mark_bit); } else { - for (MarkBit::CellType* cell = start_cell; cell <= end_cell; cell++) { - *cell = 0; - } + Marking::ClearAllMarkBitsOfCellsContainedInRange(start_mark_bit, + end_mark_bit); } return true; @@ -3595,7 +3608,7 @@ static bool IsOnInvalidatedCodeObject(Address addr) { MarkBit mark_bit = p->markbits()->MarkBitFromIndex(Page::FastAddressToMarkbitIndex(addr)); - return mark_bit.Get(); + return Marking::IsBlackOrGrey(mark_bit); } diff --git a/src/heap/mark-compact.h b/src/heap/mark-compact.h index c6800993c0..2bd53df773 100644 --- a/src/heap/mark-compact.h +++ b/src/heap/mark-compact.h @@ -50,7 +50,10 @@ class Marking { // White markbits: 00 - this is required by the mark bit clearer. static const char* kWhiteBitPattern; - INLINE(static bool IsWhite(MarkBit mark_bit)) { return !mark_bit.Get(); } + INLINE(static bool IsWhite(MarkBit mark_bit)) { + DCHECK(!IsImpossible(mark_bit)); + return !mark_bit.Get(); + } // Grey markbits: 11 static const char* kGreyBitPattern; @@ -58,19 +61,51 @@ class Marking { return mark_bit.Get() && mark_bit.Next().Get(); } + // IsBlackOrGrey assumes that the first bit is set for black or grey + // objects. + INLINE(static bool IsBlackOrGrey(MarkBit mark_bit)) { return mark_bit.Get(); } + INLINE(static void MarkBlack(MarkBit mark_bit)) { mark_bit.Set(); mark_bit.Next().Clear(); } - INLINE(static void BlackToGrey(MarkBit markbit)) { markbit.Next().Set(); } + INLINE(static void MarkWhite(MarkBit mark_bit)) { + mark_bit.Clear(); + mark_bit.Next().Clear(); + } + + INLINE(static void BlackToWhite(MarkBit markbit)) { + DCHECK(IsBlack(markbit)); + markbit.Clear(); + } + + INLINE(static void GreyToWhite(MarkBit markbit)) { + DCHECK(IsGrey(markbit)); + markbit.Clear(); + markbit.Next().Clear(); + } + + INLINE(static void BlackToGrey(MarkBit markbit)) { + DCHECK(IsBlack(markbit)); + markbit.Next().Set(); + } INLINE(static void WhiteToGrey(MarkBit markbit)) { + DCHECK(IsWhite(markbit)); markbit.Set(); markbit.Next().Set(); } - INLINE(static void GreyToBlack(MarkBit markbit)) { markbit.Next().Clear(); } + INLINE(static void WhiteToBlack(MarkBit markbit)) { + DCHECK(IsWhite(markbit)); + markbit.Set(); + } + + INLINE(static void GreyToBlack(MarkBit markbit)) { + DCHECK(IsGrey(markbit)); + markbit.Next().Clear(); + } INLINE(static void BlackToGrey(HeapObject* obj)) { BlackToGrey(MarkBitFrom(obj)); @@ -81,6 +116,10 @@ class Marking { markbit.Next().Set(); } + static void SetAllMarkBitsInRange(MarkBit start, MarkBit end); + static void ClearAllMarkBitsOfCellsContainedInRange(MarkBit start, + MarkBit end); + void TransferMark(Address old_start, Address new_start); #ifdef DEBUG diff --git a/src/heap/objects-visiting-inl.h b/src/heap/objects-visiting-inl.h index 010bbfa1e5..ea18b2e9bb 100644 --- a/src/heap/objects-visiting-inl.h +++ b/src/heap/objects-visiting-inl.h @@ -680,7 +680,7 @@ bool StaticMarkingVisitor::IsFlushable(Heap* heap, // Code is either on stack, in compilation cache or referenced // by optimized version of function. MarkBit code_mark = Marking::MarkBitFrom(function->code()); - if (code_mark.Get()) { + if (Marking::IsBlackOrGrey(code_mark)) { return false; } @@ -709,7 +709,7 @@ bool StaticMarkingVisitor::IsFlushable( // Code is either on stack, in compilation cache or referenced // by optimized version of function. MarkBit code_mark = Marking::MarkBitFrom(shared_info->code()); - if (code_mark.Get()) { + if (Marking::IsBlackOrGrey(code_mark)) { return false; } diff --git a/src/heap/spaces.cc b/src/heap/spaces.cc index d1e672adcf..59a8e1358f 100644 --- a/src/heap/spaces.cc +++ b/src/heap/spaces.cc @@ -2971,8 +2971,8 @@ void LargeObjectSpace::FreeUnmarkedObjects() { // pointer object is this big. bool is_pointer_object = object->IsFixedArray(); MarkBit mark_bit = Marking::MarkBitFrom(object); - if (mark_bit.Get()) { - mark_bit.Clear(); + if (Marking::IsBlackOrGrey(mark_bit)) { + Marking::BlackToWhite(mark_bit); Page::FromAddress(object->address())->ResetProgressBar(); Page::FromAddress(object->address())->ResetLiveBytes(); previous = current; @@ -3127,7 +3127,7 @@ void Page::Print() { unsigned mark_size = 0; for (HeapObject* object = objects.Next(); object != NULL; object = objects.Next()) { - bool is_marked = Marking::MarkBitFrom(object).Get(); + bool is_marked = Marking::IsBlackOrGrey(Marking::MarkBitFrom(object)); PrintF(" %c ", (is_marked ? '!' : ' ')); // Indent a little. if (is_marked) { mark_size += heap()->GcSafeSizeOfOldObjectFunction()(object); diff --git a/src/heap/spaces.h b/src/heap/spaces.h index fce9677213..cd5e9f2c7b 100644 --- a/src/heap/spaces.h +++ b/src/heap/spaces.h @@ -104,19 +104,15 @@ class MarkBit { inline MarkBit(CellType* cell, CellType mask) : cell_(cell), mask_(mask) {} - inline CellType* cell() { return cell_; } - inline CellType mask() { return mask_; } - #ifdef DEBUG bool operator==(const MarkBit& other) { return cell_ == other.cell_ && mask_ == other.mask_; } #endif - inline void Set() { *cell_ |= mask_; } - inline bool Get() { return (*cell_ & mask_) != 0; } - inline void Clear() { *cell_ &= ~mask_; } - + private: + inline CellType* cell() { return cell_; } + inline CellType mask() { return mask_; } inline MarkBit Next() { CellType new_mask = mask_ << 1; @@ -127,9 +123,14 @@ class MarkBit { } } - private: + inline void Set() { *cell_ |= mask_; } + inline bool Get() { return (*cell_ & mask_) != 0; } + inline void Clear() { *cell_ &= ~mask_; } + CellType* cell_; CellType mask_; + + friend class Marking; }; diff --git a/src/heap/store-buffer.cc b/src/heap/store-buffer.cc index c07ffbfe64..a73e0fa577 100644 --- a/src/heap/store-buffer.cc +++ b/src/heap/store-buffer.cc @@ -382,7 +382,8 @@ void StoreBuffer::ClearInvalidStoreBufferEntries() { LargeObjectIterator it(heap_->lo_space()); for (HeapObject* object = it.Next(); object != NULL; object = it.Next()) { MemoryChunk* chunk = MemoryChunk::FromAddress(object->address()); - if (chunk->scan_on_scavenge() && !Marking::MarkBitFrom(object).Get()) { + if (chunk->scan_on_scavenge() && + Marking::IsWhite(Marking::MarkBitFrom(object))) { chunk->set_scan_on_scavenge(false); } }