Permalink
Browse files

DO NOT MERGE: Optimize IndirectRefTable.

Removed unused debugging code that maintained pointers to the
4 previous objects in each slot.

Promoted the serial number property of a slot to be first class.
This is a sufficiently cheap and useful check to perform that we
might as well always do it.

Changed get() and remove() so they check the validity of the
references at the same time as they retrieve them.  They're
also a bit smarter about checking the reference kind given
that they can assume that the table will only contain references
of one kind.  The checks are now much more consistent.
For example, remove() used to check whether an index was
stale only in the case where it was not removing the top entry
(now it always checks).

Made add() return NULL instead of dvmAbort()'ing in the case
where the table overflowed and ensure we don't blow away the
table in that cases.  This change resolves an issue with
TestIndirectRefTable.cpp which deliberately overflows the table
and expects add() to return NULL (as documented!).  As it
happens, the add() method is called in exactly 3 places in
Jni.cpp.  In each of those cases, the code was written to
handle a NULL result and only in the case of adding a weak
global reference did it not abort.  Fixed the weak global
reference case to be consistent with the others.

Changed the signature of contains() to take an Object* since
that's what we're actually looking for inside the table.

Added a couple of calls to dump() in TestIndirectRefTable.cpp
for visual inspection of its correctness.

Performance as measured by TestIndirectRefTable on same hardware.

Old implementation:
- Add/remove 100 objects FIFO order, 100000 iterations, 0.023ms / iteration
- Add/remove 100 objects LIFO order, 100000 iterations, 0.020ms / iteration
- Get 100 objects, 100000 iterations, 0.009ms / iteration

New implementation:
- Add/remove 100 objects FIFO order, 100000 iterations, 0.010ms / iteration
- Add/remove 100 objects LIFO order, 100000 iterations, 0.009ms / iteration
- Get 100 objects, 100000 iterations, 0.002ms / iteration

Cherry-pick from master.

Conflicts:

	vm/IndirectRefTable.cpp

Change-Id: I157f3c1ba598137222878b8e6a5890efb744fe76
  • Loading branch information...
1 parent f4948cc commit 68ecde1134a8d05803552fcd82994053ada3e80d @j9brown j9brown committed with Christopher Tate Oct 27, 2011
Showing with 160 additions and 189 deletions.
  1. +127 −136 vm/IndirectRefTable.cpp
  2. +27 −52 vm/IndirectRefTable.h
  3. +3 −1 vm/Jni.cpp
  4. +3 −0 vm/test/TestIndirectRefTable.cpp
View
263 vm/IndirectRefTable.cpp
@@ -34,19 +34,14 @@ bool IndirectRefTable::init(size_t initialCount,
assert(initialCount <= maxCount);
assert(desiredKind != kIndirectKindInvalid);
- table_ = (Object**) malloc(initialCount * sizeof(Object*));
+ table_ = (IndirectRefSlot*) malloc(initialCount * sizeof(IndirectRefSlot));
if (table_ == NULL) {
return false;
}
#ifndef NDEBUG
- memset(table_, 0xd1, initialCount * sizeof(Object*));
+ memset(table_, 0xd1, initialCount * sizeof(IndirectRefSlot));
#endif
- slot_data_ = (IndirectRefSlot*) calloc(initialCount, sizeof(IndirectRefSlot));
- if (slot_data_ == NULL) {
- return false;
- }
-
segmentState.all = IRT_FIRST_SEGMENT;
alloc_entries_ = initialCount;
max_entries_ = maxCount;
@@ -61,28 +56,10 @@ bool IndirectRefTable::init(size_t initialCount,
void IndirectRefTable::destroy()
{
free(table_);
- free(slot_data_);
table_ = NULL;
- slot_data_ = NULL;
alloc_entries_ = max_entries_ = -1;
}
-/*
- * Make sure that the entry at "idx" is correctly paired with "iref".
- */
-bool IndirectRefTable::checkEntry(const char* what, IndirectRef iref, int idx) const
-{
- Object* obj = table_[idx];
- IndirectRef checkRef = toIndirectRef(obj, idx);
- if (checkRef != iref) {
- LOGE("JNI ERROR (app bug): attempt to %s stale %s reference %p (should be %p)",
- what, indirectRefKindToString(kind_), iref, checkRef);
- abortMaybe();
- return false;
- }
- return true;
-}
-
IndirectRef IndirectRefTable::add(u4 cookie, Object* obj)
{
IRTSegmentState prevState;
@@ -95,121 +72,128 @@ IndirectRef IndirectRefTable::add(u4 cookie, Object* obj)
assert(alloc_entries_ <= max_entries_);
assert(segmentState.parts.numHoles >= prevState.parts.numHoles);
- if (topIndex == alloc_entries_) {
- /* reached end of allocated space; did we hit buffer max? */
- if (topIndex == max_entries_) {
- LOGE("JNI ERROR (app bug): %s reference table overflow (max=%d)",
- indirectRefKindToString(kind_), max_entries_);
- dump(indirectRefKindToString(kind_));
- dvmAbort();
- }
-
- size_t newSize = alloc_entries_ * 2;
- if (newSize > max_entries_) {
- newSize = max_entries_;
- }
- assert(newSize > alloc_entries_);
-
- table_ = (Object**) realloc(table_, newSize * sizeof(Object*));
- slot_data_ = (IndirectRefSlot*) realloc(slot_data_, newSize * sizeof(IndirectRefSlot));
- if (table_ == NULL || slot_data_ == NULL) {
- LOGE("JNI ERROR (app bug): unable to expand %s reference table (from %d to %d, max=%d)",
- indirectRefKindToString(kind_),
- alloc_entries_, newSize, max_entries_);
- dump(indirectRefKindToString(kind_));
- dvmAbort();
- }
-
- // Clear the newly-allocated slot_data_ elements.
- memset(slot_data_ + alloc_entries_, 0, (newSize - alloc_entries_) * sizeof(IndirectRefSlot));
-
- alloc_entries_ = newSize;
- }
-
/*
* We know there's enough room in the table. Now we just need to find
* the right spot. If there's a hole, find it and fill it; otherwise,
* add to the end of the list.
*/
IndirectRef result;
+ IndirectRefSlot* slot;
int numHoles = segmentState.parts.numHoles - prevState.parts.numHoles;
if (numHoles > 0) {
assert(topIndex > 1);
- /* find the first hole; likely to be near the end of the list */
- Object** pScan = &table_[topIndex - 1];
- assert(*pScan != NULL);
- while (*--pScan != NULL) {
- assert(pScan >= table_ + prevState.parts.topIndex);
+ /* find the first hole; likely to be near the end of the list,
+ * we know the item at the topIndex is not a hole */
+ slot = &table_[topIndex - 1];
+ assert(slot->obj != NULL);
+ while ((--slot)->obj != NULL) {
+ assert(slot >= table_ + prevState.parts.topIndex);
}
- updateSlotAdd(obj, pScan - table_);
- result = toIndirectRef(obj, pScan - table_);
- *pScan = obj;
segmentState.parts.numHoles--;
} else {
- /* add to the end */
- updateSlotAdd(obj, topIndex);
- result = toIndirectRef(obj, topIndex);
- table_[topIndex++] = obj;
+ /* add to the end, grow if needed */
+ if (topIndex == alloc_entries_) {
+ /* reached end of allocated space; did we hit buffer max? */
+ if (topIndex == max_entries_) {
+ LOGE("JNI ERROR (app bug): %s reference table overflow (max=%d)",
+ indirectRefKindToString(kind_), max_entries_);
+ return NULL;
+ }
+
+ size_t newSize = alloc_entries_ * 2;
+ if (newSize > max_entries_) {
+ newSize = max_entries_;
+ }
+ assert(newSize > alloc_entries_);
+
+ IndirectRefSlot* newTable =
+ (IndirectRefSlot*) realloc(table_, newSize * sizeof(IndirectRefSlot));
+ if (table_ == NULL) {
+ LOGE("JNI ERROR (app bug): unable to expand %s reference table "
+ "(from %d to %d, max=%d)",
+ indirectRefKindToString(kind_),
+ alloc_entries_, newSize, max_entries_);
+ return NULL;
+ }
+
+ alloc_entries_ = newSize;
+ table_ = newTable;
+ }
+ slot = &table_[topIndex++];
segmentState.parts.topIndex = topIndex;
}
+ slot->obj = obj;
+ slot->serial = nextSerial(slot->serial);
+ result = toIndirectRef(slot - table_, slot->serial, kind_);
+
assert(result != NULL);
return result;
}
/*
- * Verify that the indirect table lookup is valid.
+ * Get the referent of an indirect ref from the table.
*
- * Returns "false" if something looks bad.
+ * Returns kInvalidIndirectRefObject if iref is invalid.
*/
-bool IndirectRefTable::getChecked(IndirectRef iref) const
-{
- if (iref == NULL) {
- LOGW("Attempt to look up NULL %s reference", indirectRefKindToString(kind_));
- return false;
- }
- if (indirectRefKind(iref) == kIndirectKindInvalid) {
- LOGE("JNI ERROR (app bug): invalid %s reference %p",
- indirectRefKindToString(kind_), iref);
- abortMaybe();
- return false;
+Object* IndirectRefTable::get(IndirectRef iref) const {
+ IndirectRefKind kind = indirectRefKind(iref);
+ if (kind != kind_) {
+ if (iref == NULL) {
+ LOGW("Attempt to look up NULL %s reference", indirectRefKindToString(kind_));
+ return kInvalidIndirectRefObject;
+ }
+ if (kind == kIndirectKindInvalid) {
+ LOGE("JNI ERROR (app bug): invalid %s reference %p",
+ indirectRefKindToString(kind_), iref);
+ abortMaybe();
+ return kInvalidIndirectRefObject;
+ }
+ // References of the requested kind cannot appear within this table.
+ return kInvalidIndirectRefObject;
}
- int topIndex = segmentState.parts.topIndex;
- int idx = extractIndex(iref);
- if (idx >= topIndex) {
+ u4 topIndex = segmentState.parts.topIndex;
+ u4 index = extractIndex(iref);
+ if (index >= topIndex) {
/* bad -- stale reference? */
LOGE("JNI ERROR (app bug): accessed stale %s reference %p (index %d in a table of size %d)",
- indirectRefKindToString(kind_), iref, idx, topIndex);
+ indirectRefKindToString(kind_), iref, index, topIndex);
abortMaybe();
- return false;
+ return kInvalidIndirectRefObject;
}
- if (table_[idx] == NULL) {
+ Object* obj = table_[index].obj;
+ if (obj == NULL) {
LOGI("JNI ERROR (app bug): accessed deleted %s reference %p",
indirectRefKindToString(kind_), iref);
abortMaybe();
- return false;
+ return kInvalidIndirectRefObject;
}
- if (!checkEntry("use", iref, idx)) {
- return false;
+ u4 serial = extractSerial(iref);
+ if (serial != table_[index].serial) {
+ LOGE("JNI ERROR (app bug): attempt to use stale %s reference %p",
+ indirectRefKindToString(kind_), iref);
+ abortMaybe();
+ return kInvalidIndirectRefObject;
}
- return true;
+ return obj;
}
-static int linearScan(IndirectRef iref, int bottomIndex, int topIndex, Object** table) {
+static int findObject(const Object* obj, int bottomIndex, int topIndex,
+ const IndirectRefSlot* table) {
for (int i = bottomIndex; i < topIndex; ++i) {
- if (table[i] == reinterpret_cast<Object*>(iref)) {
+ if (table[i].obj == obj) {
return i;
}
}
return -1;
}
-bool IndirectRefTable::contains(IndirectRef iref) const {
- return linearScan(iref, 0, segmentState.parts.topIndex, table_) != -1;
+bool IndirectRefTable::contains(const Object* obj) const {
+ return findObject(obj, 0, segmentState.parts.topIndex, table_) >= 0;
}
/*
@@ -229,52 +213,61 @@ bool IndirectRefTable::remove(u4 cookie, IndirectRef iref)
{
IRTSegmentState prevState;
prevState.all = cookie;
- int topIndex = segmentState.parts.topIndex;
- int bottomIndex = prevState.parts.topIndex;
+ u4 topIndex = segmentState.parts.topIndex;
+ u4 bottomIndex = prevState.parts.topIndex;
assert(table_ != NULL);
assert(alloc_entries_ <= max_entries_);
assert(segmentState.parts.numHoles >= prevState.parts.numHoles);
- int idx = extractIndex(iref);
- bool workAroundAppJniBugs = false;
-
- if (indirectRefKind(iref) == kIndirectKindInvalid && gDvmJni.workAroundAppJniBugs) {
- idx = linearScan(iref, bottomIndex, topIndex, table_);
- workAroundAppJniBugs = true;
- if (idx == -1) {
+ IndirectRefKind kind = indirectRefKind(iref);
+ u4 index;
+ if (kind == kind_) {
+ index = extractIndex(iref);
+ if (index < bottomIndex) {
+ /* wrong segment */
+ LOGV("Attempt to remove index outside index area (%ud vs %ud-%ud)",
+ index, bottomIndex, topIndex);
+ return false;
+ }
+ if (index >= topIndex) {
+ /* bad -- stale reference? */
+ LOGD("Attempt to remove invalid index %ud (bottom=%ud top=%ud)",
+ index, bottomIndex, topIndex);
+ return false;
+ }
+ if (table_[index].obj == NULL) {
+ LOGD("Attempt to remove cleared %s reference %p",
+ indirectRefKindToString(kind_), iref);
+ return false;
+ }
+ u4 serial = extractSerial(iref);
+ if (table_[index].serial != serial) {
+ LOGD("Attempt to remove stale %s reference %p",
+ indirectRefKindToString(kind_), iref);
+ return false;
+ }
+ } else if (kind == kIndirectKindInvalid && gDvmJni.workAroundAppJniBugs) {
+ // reference looks like a pointer, scan the table to find the index
+ int i = findObject(reinterpret_cast<Object*>(iref), bottomIndex, topIndex, table_);
+ if (i < 0) {
LOGW("trying to work around app JNI bugs, but didn't find %p in table!", iref);
return false;
}
- }
-
- if (idx < bottomIndex) {
- /* wrong segment */
- LOGV("Attempt to remove index outside index area (%d vs %d-%d)",
- idx, bottomIndex, topIndex);
- return false;
- }
- if (idx >= topIndex) {
- /* bad -- stale reference? */
- LOGD("Attempt to remove invalid index %d (bottom=%d top=%d)",
- idx, bottomIndex, topIndex);
+ index = i;
+ } else {
+ // References of the requested kind cannot appear within this table.
return false;
}
- if (idx == topIndex-1) {
+ if (index == topIndex - 1) {
// Top-most entry. Scan up and consume holes.
-
- if (workAroundAppJniBugs == false && !checkEntry("remove", iref, idx)) {
- return false;
- }
-
- table_[idx] = NULL;
int numHoles = segmentState.parts.numHoles - prevState.parts.numHoles;
if (numHoles != 0) {
while (--topIndex > bottomIndex && numHoles != 0) {
LOGV("+++ checking for hole at %d (cookie=0x%08x) val=%p",
- topIndex-1, cookie, table_[topIndex-1]);
- if (table_[topIndex-1] != NULL) {
+ topIndex-1, cookie, table_[topIndex-1].obj);
+ if (table_[topIndex-1].obj != NULL) {
break;
}
LOGV("+++ ate hole at %d", topIndex-1);
@@ -292,17 +285,9 @@ bool IndirectRefTable::remove(u4 cookie, IndirectRef iref)
* entry to prevent somebody from deleting it twice and screwing up
* the hole count.
*/
- if (table_[idx] == NULL) {
- LOGV("--- WEIRD: removing null entry %d", idx);
- return false;
- }
- if (workAroundAppJniBugs == false && !checkEntry("remove", iref, idx)) {
- return false;
- }
-
- table_[idx] = NULL;
+ table_[index].obj = NULL;
segmentState.parts.numHoles++;
- LOGV("+++ left hole at %d, holes=%d", idx, segmentState.parts.numHoles);
+ LOGV("+++ left hole at %d, holes=%d", index, segmentState.parts.numHoles);
}
return true;
@@ -321,5 +306,11 @@ const char* indirectRefKindToString(IndirectRefKind kind)
void IndirectRefTable::dump(const char* descr) const
{
- dvmDumpReferenceTableContents(table_, capacity(), descr);
+ size_t count = capacity();
+ Object** copy = new Object*[count];
+ for (size_t i = 0; i < count; i++) {
+ copy[i] = table_[i].obj;
+ }
+ dvmDumpReferenceTableContents(copy, count, descr);
+ delete[] copy;
}
View
79 vm/IndirectRefTable.h
@@ -82,6 +82,8 @@
* memory accesses on add/get. It will catch additional problems, e.g.:
* create iref1 for obj, delete iref1, create iref2 for same obj, lookup
* iref1. A pattern based on object bits will miss this.
+ *
+ * For now, we use a serial number.
*/
typedef void* IndirectRef;
@@ -112,13 +114,11 @@ INLINE IndirectRefKind indirectRefKind(IndirectRef iref)
}
/*
- * Extended debugging structure. We keep a parallel array of these, one
- * per slot in the table.
+ * Information we store for each slot in the reference table.
*/
-#define kIRTPrevCount 4
struct IndirectRefSlot {
- u4 serial; /* slot serial */
- Object* previous[kIRTPrevCount];
+ Object* obj; /* object pointer itself, NULL if the slot is unused */
+ u4 serial; /* slot serial number */
};
/* use as initial value for "cookie", and when table has only one segment */
@@ -201,9 +201,8 @@ union IRTSegmentState {
class iref_iterator {
public:
- explicit iref_iterator(Object** table, size_t i, size_t capacity)
- : table_(table), i_(i), capacity_(capacity)
- {
+ explicit iref_iterator(IndirectRefSlot* table, size_t i, size_t capacity) :
+ table_(table), i_(i), capacity_(capacity) {
skipNullsAndTombstones();
}
@@ -214,24 +213,23 @@ class iref_iterator {
}
Object** operator*() {
- return &table_[i_];
+ return &table_[i_].obj;
}
bool equals(const iref_iterator& rhs) const {
return (i_ == rhs.i_ && table_ == rhs.table_);
}
- size_t to_i() const { return i_; }
-
private:
void skipNullsAndTombstones() {
// We skip NULLs and tombstones. Clients don't want to see implementation details.
- while (i_ < capacity_ && (table_[i_] == NULL || table_[i_] == kClearedJniWeakGlobal)) {
+ while (i_ < capacity_ && (table_[i_].obj == NULL
+ || table_[i_].obj == kClearedJniWeakGlobal)) {
++i_;
}
}
- Object** table_;
+ IndirectRefSlot* table_;
size_t i_;
size_t capacity_;
};
@@ -254,11 +252,9 @@ struct IndirectRefTable {
* uses offsetof, since private member data makes us non-POD.
*/
/* bottom of the stack */
- Object** table_;
+ IndirectRefSlot* table_;
/* bit mask, ORed into all irefs */
IndirectRefKind kind_;
- /* extended debugging info */
- IndirectRefSlot* slot_data_;
/* #of entries we have space for */
size_t alloc_entries_;
/* max #of entries allowed */
@@ -282,15 +278,12 @@ struct IndirectRefTable {
*
* Returns kInvalidIndirectRefObject if iref is invalid.
*/
- Object* get(IndirectRef iref) const {
- if (!getChecked(iref)) {
- return kInvalidIndirectRefObject;
- }
- return table_[extractIndex(iref)];
- }
+ Object* get(IndirectRef iref) const;
- // TODO: only used for workAroundAppJniBugs support.
- bool contains(IndirectRef iref) const;
+ /*
+ * Returns true if the table contains a reference to this object.
+ */
+ bool contains(const Object* obj) const;
/*
* Remove an existing entry.
@@ -351,42 +344,24 @@ struct IndirectRefTable {
}
private:
- /*
- * Extract the table index from an indirect reference.
- */
- static u4 extractIndex(IndirectRef iref) {
+ static inline u4 extractIndex(IndirectRef iref) {
u4 uref = (u4) iref;
return (uref >> 2) & 0xffff;
}
- /*
- * The object pointer itself is subject to relocation in some GC
- * implementations, so we shouldn't really be using it here.
- */
- IndirectRef toIndirectRef(Object* obj, u4 tableIndex) const {
- assert(tableIndex < 65536);
- u4 serialChunk = slot_data_[tableIndex].serial;
- u4 uref = serialChunk << 20 | (tableIndex << 2) | kind_;
- return (IndirectRef) uref;
+ static inline u4 extractSerial(IndirectRef iref) {
+ u4 uref = (u4) iref;
+ return uref >> 20;
}
- /*
- * Update extended debug info when an entry is added.
- *
- * We advance the serial number, invalidating any outstanding references to
- * this slot.
- */
- void updateSlotAdd(Object* obj, int slot) {
- if (slot_data_ != NULL) {
- IndirectRefSlot* pSlot = &slot_data_[slot];
- pSlot->serial++;
- pSlot->previous[pSlot->serial % kIRTPrevCount] = obj;
- }
+ static inline u4 nextSerial(u4 serial) {
+ return (serial + 1) & 0xfff;
}
- /* extra debugging checks */
- bool getChecked(IndirectRef) const;
- bool checkEntry(const char*, IndirectRef, int) const;
+ static inline IndirectRef toIndirectRef(u4 index, u4 serial, IndirectRefKind kind) {
+ assert(index < 65536);
+ return reinterpret_cast<IndirectRef>((serial << 20) | (index << 2) | kind);
+ }
};
#endif // DALVIK_INDIRECTREFTABLE_H_
View
4 vm/Jni.cpp
@@ -558,7 +558,9 @@ static jobject addWeakGlobalReference(Object* obj) {
IndirectRefTable *table = &gDvm.jniWeakGlobalRefTable;
jobject jobj = (jobject) table->add(IRT_FIRST_SEGMENT, obj);
if (jobj == NULL) {
+ gDvm.jniWeakGlobalRefTable.dump("JNI weak global");
LOGE("Failed adding to JNI weak global ref table (%zd entries)", table->capacity());
+ dvmAbort();
}
return jobj;
}
@@ -706,7 +708,7 @@ jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj) {
if (obj == reinterpret_cast<Object*>(jobj) && gDvmJni.workAroundAppJniBugs) {
// If we're handing out direct pointers, check whether 'jobj' is a direct reference
// to a local reference.
- return getLocalRefTable(env)->contains(jobj) ? JNILocalRefType : JNIInvalidRefType;
+ return getLocalRefTable(env)->contains(obj) ? JNILocalRefType : JNIInvalidRefType;
} else if (obj == kInvalidIndirectRefObject) {
return JNIInvalidRefType;
} else {
View
3 vm/test/TestIndirectRefTable.cpp
@@ -281,12 +281,14 @@ static bool basicTest()
LOGE("Expected %d entries, found %d", kTableMax, irt.capacity());
goto bail;
}
+ irt.dump("table with 20 entries, all filled");
for (i = 0; i < kTableMax-1; i++) {
if (!irt.remove(cookie, manyRefs[i])) {
LOGE("multi-remove failed at %d", i);
goto bail;
}
}
+ irt.dump("table with 20 entries, 19 of them holes");
/* because of removal order, should have 20 entries, 19 of them holes */
if (irt.capacity() != (size_t)kTableMax) {
LOGE("Expected %d entries (with holes), found %d",
@@ -302,6 +304,7 @@ static bool basicTest()
goto bail;
}
+ /* Done */
DBUG_MSG("+++ basic test complete\n");
result = true;

0 comments on commit 68ecde1

Please sign in to comment.