From 21aed89e88703eef1f437969d05fbab1c149b0eb Mon Sep 17 00:00:00 2001
From: Michael Goulet <michael@errs.io>
Date: Sun, 3 Mar 2019 12:17:14 -0800
Subject: [PATCH] Fix unnecessary pinning in `btreefile.LeafPage`
We pin an underlying page N times too many whenever we instantiate a new
LeafPage class, due to the constructor of the BTreeFilePageTuple. What we
want is a tuple that acts like a _weak_ reference to the underlying
page, and only make it a _strong_ reference whenever this tuple escapes
the scope of the btree-internal classes (e.g. in `BTreeFile.addTuple`).
Right now, I just add an `unpin()` call in the constructor of the Tuple,
and then pin it whenever I know that it escapes the scope of the btree
package. Maybe there's a more graceful way of doing it. I have no idea.
---
.../nanodb/plannodes/MaterializeNode.java | 2 +-
.../storage/btreefile/BTreeFilePageTuple.java | 8 +++++++
.../storage/btreefile/BTreeFileVerifier.java | 4 ++--
.../storage/btreefile/BTreeTupleFile.java | 23 +++++++++++++++----
4 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/src/main/java/edu/caltech/nanodb/plannodes/MaterializeNode.java b/src/main/java/edu/caltech/nanodb/plannodes/MaterializeNode.java
index 6768762..37c3aa4 100644
--- a/src/main/java/edu/caltech/nanodb/plannodes/MaterializeNode.java
+++ b/src/main/java/edu/caltech/nanodb/plannodes/MaterializeNode.java
@@ -112,7 +112,7 @@ public class MaterializeNode extends PlanNode {
if (tup != null) {
if (tup.isDiskBacked()) {
// Make an in-memory version of the tuple we can cache.
- Tuple copy = new TupleLiteral(tup);
+ Tuple copy = TupleLiteral.fromTuple(tup);
tup.unpin();
tup = copy;
}
diff --git a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFilePageTuple.java b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFilePageTuple.java
index 751eef7..dc43379 100644
--- a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFilePageTuple.java
+++ b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFilePageTuple.java
@@ -58,6 +58,14 @@ public class BTreeFilePageTuple extends PageTuple {
"tupleIndex must be at least 0, got " + tupleIndex);
}
+ unpin(); // TODO(michael): This is gross. I know.
+ // What we really want here is a tuple that acts like a "weak reference" to its
+ // underlying page. This is because we actually instantiate these tuples inside
+ // the page itself (well, inside of LeafPage), so we end up having the page
+ // pinned N times more than the actual references that escape outside of the
+ // internal BT data structures. We will re-pin the tuples in the outward-facing
+ // methods in `BTreeTupleFile`.
+
this.tupleIndex = tupleIndex;
}
diff --git a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFileVerifier.java b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFileVerifier.java
index 2892ef1..2d4fee5 100644
--- a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFileVerifier.java
+++ b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeFileVerifier.java
@@ -260,7 +260,7 @@ public class BTreeFileVerifier {
ArrayList<TupleLiteral> keys = new ArrayList<>(numKeys);
if (numKeys > 1) {
Tuple prevKey = inner.getKey(0);
- keys.add(new TupleLiteral(prevKey));
+ keys.add(TupleLiteral.fromTuple(prevKey));
if (parentLeftKey != null) {
int cmp = TupleComparator.compareTuples(parentLeftKey, prevKey);
@@ -275,7 +275,7 @@ public class BTreeFileVerifier {
for (int k = 1; k < numKeys; k++) {
Tuple key = inner.getKey(k);
- keys.add(new TupleLiteral(key));
+ keys.add(TupleLiteral.fromTuple(key));
int cmp = TupleComparator.compareTuples(prevKey, key);
if (cmp == 0) {
diff --git a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeTupleFile.java b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeTupleFile.java
index 01ab84d..b237bf8 100644
--- a/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeTupleFile.java
+++ b/src/main/java/edu/caltech/nanodb/storage/btreefile/BTreeTupleFile.java
@@ -190,6 +190,7 @@ public class BTreeTupleFile implements SequentialTupleFile {
if (leaf != null && leaf.getNumTuples() > 0)
tup = leaf.getTuple(0);
+ tup.pin();
return tup;
}
@@ -256,10 +257,16 @@ public class BTreeTupleFile implements SequentialTupleFile {
logger.error(String.format(
"Next leaf node %d has no entries?!", nextPageNo));
}
+
+ dbPage.unpin();
}
}
}
+ if (nextTuple != null) {
+ nextTuple.pin();
+ }
+
return nextTuple;
}
@@ -281,8 +288,11 @@ public class BTreeTupleFile implements SequentialTupleFile {
LeafPage leaf = new LeafPage(dbPage, schema);
for (int i = 0; i < leaf.getNumTuples(); i++) {
BTreeFilePageTuple tup = leaf.getTuple(i);
- if (tup.getOffset() == fpOffset)
+
+ if (tup.getOffset() == fpOffset) {
+ tup.pin();
return tup;
+ }
// Tuple offsets within a page will be monotonically increasing.
if (tup.getOffset() > fpOffset)
@@ -320,6 +330,7 @@ public class BTreeTupleFile implements SequentialTupleFile {
if (cmp == 0) {
// Found it!
+ tup.pin();
return tup;
}
else if (cmp > 0) {
@@ -372,8 +383,10 @@ public class BTreeTupleFile implements SequentialTupleFile {
for (int i = 0; i < leaf.getNumTuples(); i++) {
BTreeFilePageTuple tup = leaf.getTuple(i);
int cmp = TupleComparator.comparePartialTuples(tup, searchKey);
- if (cmp > 0)
+ if (cmp > 0) {
+ tup.pin();
return tup; // Found it!
+ }
}
leaf.getDBPage().unpin();
@@ -397,10 +410,12 @@ public class BTreeTupleFile implements SequentialTupleFile {
if (tup instanceof TupleLiteral)
tupLit = (TupleLiteral) tup;
else
- tupLit = new TupleLiteral(tup);
+ tupLit = TupleLiteral.fromTuple(tup);
tupLit.setStorageSize(PageTuple.getTupleStorageSize(schema, tupLit));
- return leafPageOps.addTuple(leaf, tupLit, pagePath);
+ BTreeFilePageTuple bTup = leafPageOps.addTuple(leaf, tupLit, pagePath);
+ bTup.pin();
+ return bTup;
}
--
GitLab