RFC: Clean up transaction oracle as we go #1198

damz · 2020-01-15T22:02:54Z

In the current implementation, if you happen to always have at least one write transaction open the memory usage of the transaction oracle is unbounded. It is actually relatively easy to hit when batch importing data. If you have more than one WriteBatch active during the import the transaction oracle will never be cleaned up.

This is a RFC on an approach to fix this. The core idea is to:

Avoid increasing contention on purely read transactions; so only clean up the transaction oracle when write transactions are committed even if technically we could free memory sooner;
Split the big oracle.commit map into one map per previously committed transaction; (this allows Go to release memory sooner than when performing deletes on a single map);
Take advantage of the fact that we have acquired the oracle lock in oracle.newCommitTs to do the cleanup

I am assuming here that the number of committed-but-still-tracked transactions is small, which makes an implementation based on a simple slice reasonable. If that's not the case we will need some form of a sorted data-structure (i.e. a b-tree) here.

Comments welcome.

This change is

coveralls · 2020-01-16T04:17:19Z

Coverage decreased (-0.1%) to 69.874% when pulling e8eba91 on damz:pr/oracle-cleanup into 3747be5 on dgraph-io:master.

jarifibrahim

This is great work @damz . I've added some comments to better understand the code.

jarifibrahim · 2020-01-27T10:55:49Z

txn.go

 		// A commit at the read timestamp is expected.
 		// But, any commit after the read timestamp should cause a conflict.
-		if ts, has := o.commits[ro]; has && ts > txn.readTs {
-			return true
+		if committedTxn.ts <= txn.readTs {


Please add a comment here

If the committedTxn.ts is less than txn.readTs that implies that the committedTxn finished before the current transaction started. We don't need to check for conflict in that case.

jarifibrahim · 2020-01-27T11:00:02Z

txn.go

@@ -184,12 +177,50 @@ func (o *oracle) newCommitTs(txn *Txn) uint64 {
 		ts = txn.commitTs
 	}

-	for _, w := range txn.writes {
-		o.commits[w] = ts // Update the commitTs.
+	if ts > o.lastCleanupTs {


Do we need this check? I'm wondering in what case can this check be false.

In non-managed mode, ts will always be greater than the o.lastCleanupTs since we always get increasing ts.
In managed mode, the user could accidentally give an incorrect txn.commitTs. In that case we should complain about it.

I think we should remove the if and add y.AssertTruef(ts > o.lastCleanupTs, "ts: %d should not be less than lastCleanup: %d", ts, o.lastCleanup)

You are right that this is suspicious-looking. I think I was under the (mistaken) assumption that there could be case where the commit timestamp would not increase, but obviously that would break assumptions all over the place.

Yes. If the new timestamp is smaller than the previous one, it would mess up the look-ups. We assume that newer values with be at a higher level (level 0, level 1, etc) with higher timestamps.

Let's just complain to the user in that case. This could potentially mean there's something seriously wrong with badger or whoever is using badger. We shouldn't quitely continue here.

jarifibrahim · 2020-01-27T11:02:07Z

txn.go

-	reads  []uint64 // contains fingerprints of keys read.
-	writes []uint64 // contains fingerprints of keys written.
+	update bool                // update is used to conditionally keep track of reads.
+	reads  []uint64            // contains fingerprints of keys read.


The reads is a slice here which means that if we keep reading the same key again and again, it will be added to the reads list which could cause OOM error. This can be fixed separately. I know it's not being introduced in this PR.

jarifibrahim · 2020-01-27T11:11:45Z

txn.go

@@ -51,17 +49,21 @@ type oracle struct {
 	readMark  *y.WaterMark // Used by DB.

 	// commits stores a key fingerprint and latest commit counter for it.


This comment needs to be updated.

jarifibrahim · 2020-01-27T11:57:53Z

txn.go

@@ -172,6 +160,11 @@ func (o *oracle) newCommitTs(txn *Txn) uint64 {
 		return 0
 	}

+	if !o.isManaged {
+		o.doneRead(txn)


I think we can remove the o.doneRead call from here because txn.Discard() will be called for every transaction and the discard method will call o.doneRead(...).

I think we can also get rid of the doneRead variable from the txn struct. The only reason it was needed was because we were calling o.doneRead() at multiple places.

The idea is to take advantage of the fact we have acquired the lock to do the clean up. For that to work we need to mark the transaction as done reading first.

jarifibrahim · 2020-01-27T14:20:56Z

txn.go

+		maxReadTs = o.readMark.DoneUntil()
+	}
+
+	if maxReadTs <= o.lastCleanupTs {


I'm curious how we could end up in this condition. o.lastCleanupTs should always be less than the maxReadTs.

The < part is just defensive programming. I can replace that with an assert if you prefer.

The == part is an optimization: do not run clean up if the maxReadTs (which is the read timestamp of the oldest transaction that is still in flight) has not increased.

The < part is just defensive programming. I can replace that with an assert if you prefer.

I understand your point but I think we should complain here with a y.Assert.

The == part is an optimization: do not run clean up if the maxReadTs (which is the read timestamp of the oldest transaction that is still in flight) has not increased.

Oh, yes. That makes sense. Thanks.

stale · 2020-03-01T08:17:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-03-31T09:03:50Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2020-04-07T09:26:26Z

This issue was marked as stale and no activity has occurred since then, therefore it will now be closed. Please, reopen if the issue is still relevant.

jarifibrahim · 2020-04-07T18:10:50Z

#1275 contains an updated version of this PR.

damz requested review from ashish-goswami, jarifibrahim and manishrjain as code owners January 15, 2020 22:02

damz requested a review from a team January 15, 2020 22:02

Clean up transaction oracle as we go

e8eba91

damz force-pushed the pr/oracle-cleanup branch from e8b5093 to e8eba91 Compare January 16, 2020 04:01

jarifibrahim suggested changes Jan 27, 2020

View reviewed changes

stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Mar 1, 2020

jarifibrahim removed the status/stale The issue hasn't had activity for a while and it's marked for closing. label Mar 6, 2020

jarifibrahim mentioned this pull request Mar 18, 2020

Potential memory leak in oracle #1238

Closed

muXxer added a commit to muXxer/badger that referenced this pull request Mar 24, 2020

Resolve all comments from hypermodeinc#1198

10c0b88

muXxer added a commit to muXxer/badger that referenced this pull request Mar 24, 2020

Resolve all comments from hypermodeinc#1198

ba8ff04

muXxer mentioned this pull request Mar 24, 2020

Clean up transaction oracle as we go, take two #1275

Merged

muXxer added a commit to muXxer/badger that referenced this pull request Mar 24, 2020

Resolve all comments from hypermodeinc#1198

7f2b2ea

stale bot added the status/stale The issue hasn't had activity for a while and it's marked for closing. label Mar 31, 2020

stale bot closed this Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Clean up transaction oracle as we go #1198

RFC: Clean up transaction oracle as we go #1198

damz commented Jan 15, 2020 •

edited by manishrjain

Loading

coveralls commented Jan 16, 2020 •

edited

Loading

jarifibrahim left a comment

jarifibrahim Jan 27, 2020

jarifibrahim Jan 27, 2020

damz Jan 27, 2020

jarifibrahim Jan 31, 2020

jarifibrahim Jan 27, 2020

jarifibrahim Jan 27, 2020

jarifibrahim Jan 27, 2020

damz Jan 27, 2020

jarifibrahim Jan 27, 2020

damz Jan 27, 2020

jarifibrahim Jan 31, 2020

stale bot commented Mar 1, 2020

stale bot commented Mar 31, 2020

stale bot commented Apr 7, 2020

jarifibrahim commented Apr 7, 2020

		@@ -51,17 +49,21 @@ type oracle struct {
		readMark *y.WaterMark // Used by DB.

		// commits stores a key fingerprint and latest commit counter for it.

RFC: Clean up transaction oracle as we go #1198

RFC: Clean up transaction oracle as we go #1198

Conversation

damz commented Jan 15, 2020 • edited by manishrjain Loading

coveralls commented Jan 16, 2020 • edited Loading

jarifibrahim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Mar 1, 2020

stale bot commented Mar 31, 2020

stale bot commented Apr 7, 2020

jarifibrahim commented Apr 7, 2020

damz commented Jan 15, 2020 •

edited by manishrjain

Loading

coveralls commented Jan 16, 2020 •

edited

Loading