Fix ModifyCardsAtomic comment
The function doesn't behave as it was previously specified. Its
actual semantics are arguably ugly, but that doesn't really seem to
break anything. Just specify what it really does, and add DCHECK
to validate previously undocumented assumption.
Deduplicate the comment.
Update card_table_test, so it no longer invokes ModifyCardsAtomic
with a now disallowed visitor. This was OK, because it's applied
to a card table containing no zeroes, and hence the misbehaving
visitor didn't matter. Now we check.
Test: Treehugger.
Change-Id: Id1ba1cb39c709a33c362d4e386ddcb065bd39049
diff --git a/runtime/gc/accounting/card_table-inl.h b/runtime/gc/accounting/card_table-inl.h
index 1e7d76c..df50682 100644
--- a/runtime/gc/accounting/card_table-inl.h
+++ b/runtime/gc/accounting/card_table-inl.h
@@ -127,14 +127,6 @@
return cards_scanned;
}
-/*
- * Visitor is expected to take in a card and return the new value. When a value is modified, the
- * modify visitor is called.
- * visitor: The visitor which modifies the cards. Returns the new value for a card given an old
- * value.
- * modified: Whenever the visitor modifies a card, this visitor is called on the card. Enables
- * us to know which cards got cleared.
- */
template <typename Visitor, typename ModifiedVisitor>
inline void CardTable::ModifyCardsAtomic(uint8_t* scan_begin,
uint8_t* scan_end,
@@ -144,6 +136,7 @@
uint8_t* card_end = CardFromAddr(AlignUp(scan_end, kCardSize));
CheckCardValid(card_cur);
CheckCardValid(card_end);
+ DCHECK(visitor(kCardClean) == kCardClean);
// Handle any unaligned cards at the start.
while (!IsAligned<sizeof(intptr_t)>(card_cur) && card_cur < card_end) {
@@ -188,7 +181,8 @@
while (word_cur < word_end) {
while (true) {
expected_word = *word_cur;
- if (LIKELY(expected_word == 0)) {
+ static_assert(kCardClean == 0);
+ if (LIKELY(expected_word == 0 /* All kCardClean */ )) {
break;
}
for (size_t i = 0; i < sizeof(uintptr_t); ++i) {
diff --git a/runtime/gc/accounting/card_table.h b/runtime/gc/accounting/card_table.h
index f163898..d9daf25 100644
--- a/runtime/gc/accounting/card_table.h
+++ b/runtime/gc/accounting/card_table.h
@@ -95,12 +95,10 @@
}
/*
- * Visitor is expected to take in a card and return the new value. When a value is modified, the
- * modify visitor is called.
- * visitor: The visitor which modifies the cards. Returns the new value for a card given an old
- * value.
- * modified: Whenever the visitor modifies a card, this visitor is called on the card. Enables
- * us to know which cards got cleared.
+ * Modify cards in the range from scan_begin (inclusive) to scan_end (exclusive). Each card
+ * value v is replaced by visitor(v). Visitor() should not have side-effects.
+ * Whenever a card value is changed, modified(card_address, old_value, new_value) is invoked.
+ * For opportunistic performance reasons, this assumes that visitor(kCardClean) is kCardClean!
*/
template <typename Visitor, typename ModifiedVisitor>
void ModifyCardsAtomic(uint8_t* scan_begin,
diff --git a/runtime/gc/accounting/card_table_test.cc b/runtime/gc/accounting/card_table_test.cc
index 87965ed..12baaa4 100644
--- a/runtime/gc/accounting/card_table_test.cc
+++ b/runtime/gc/accounting/card_table_test.cc
@@ -60,7 +60,7 @@
uint8_t* HeapLimit() const {
return HeapBegin() + heap_size_;
}
- // Return a pseudo random card for an address.
+ // Return a non-zero pseudo random card for an address.
uint8_t PseudoRandomCard(const uint8_t* addr) const {
size_t offset = RoundDown(addr - heap_begin_, CardTable::kCardSize);
return 1 + offset % 254;
@@ -97,7 +97,8 @@
class UpdateVisitor {
public:
uint8_t operator()(uint8_t c) const {
- return c * 93 + 123;
+ // Must map zero to zero. Never applied to zero.
+ return c == 0 ? 0 : c * 93 + 123;
}
void operator()(uint8_t* /*card*/, uint8_t /*expected_value*/, uint8_t /*new_value*/) const {
}