RWSet performance fixes, LockFreeArray performance fixes and tests #1401

mbutrovich · 2018-06-11T18:55:42Z

LockFreeArray:

Changed functions that unconditionally returned true to be void.
Rewrote FindValid to no longer be O(n) lookups. This is used in DataTable:GetTileGroup() and should be fast.
Added Doxygen comments.
Added more tests.

TimestampOrderingTransactionManger:

Keep track of last accessed tile_group_id and if it's the same avoid going to the StorageManager’s CuckooHashMap for the TileGroup.

TransactionContext:

Simplified conditionals to what should be equivalent logic. Reduced lookups for Release mode, increased lookups in Debug mode (by moving lookups that only served for PELOTON_ASSERTs into individual lookups inside the ASSERTs)
Removed the iterator hints since Intel docs show they're not used:
https://software.intel.com/en-us/node/506175
Removed is_written_ and insert_count_ members and related logic since they were not used for anything.

@pervazea is going to help me out and grab callgrind numbers when he has time to demonstrate these improvements.

tcm-marcel · 2018-06-12T00:23:58Z

src/concurrency/transaction_context.cpp

    return;
  }
-  rw_set_.insert(rw_set_it, std::make_pair(location, RWType::READ));
+  rw_set_[location] = RWType::READ;


So we have still two lookups here, one in line 99 and one in line 103. Is it even possible to do this with a single lookup? No, because some other thread could have changed the RW set between these to lines?

As best as I can tell, based on our current semantics we can't get away from two lookups on Read, or Delete.

tcm-marcel

LGTM. I have some questions about the RW set, but maybe I just missed something again.

tcm-marcel · 2018-06-12T00:26:01Z

src/concurrency/transaction_context.cpp

+  PELOTON_ASSERT(rw_set_.count(location) == 0 ||
+                 (rw_set_[location] != RWType::DELETE &&
+                  rw_set_[location] != RWType::INS_DEL));
+  rw_set_[location] = RWType::UPDATE;


I think some functionality got lost here. If rw_type == RWType::INSERT it should not be update to RWType::UPDATE.

You're right, I'll rearrange that logic. I was trying to get away from two lookups in the case of an insert but it won't allow that.

tcm-marcel · 2018-06-12T00:29:09Z

src/concurrency/transaction_context.cpp

-      return false;
-    }
+  if (rw_set_it != rw_set_.end() && rw_set_it->second == RWType::INSERT) {
+    rw_set_it->second = RWType::INS_DEL;


I am just wondering - what does happen, if the value of rw_set_it->second changes between line 130 and line 131/133. Do we still do the right thing?

I suspect it's a race that isn't handled in the current semantics of our RWSet. The hope is that we'll eventually get rid of the INS_DEL type anyway when we stop reusing tuple slots.

tcm-marcel · 2018-06-12T00:31:18Z

test/common/lock_free_array_test.cpp

+  {
+    LockFreeArray<value_type> array;
+
+    value_type invalid_value = 6288;


Random number?

Just any sentinel value for this test that wasn't inserted will do.

Please use INVALID_OID instead.

mbutrovich · 2018-06-12T15:08:35Z

@pervazea ran a microbenchmark of this branch against master that showed a drop in calls to StorageManager::GetTileGroup (and thereby CuckooMap::Find) from 1229 to 1083.

poojanilangekar

Looks good overall. I have two main concerns.

I am not entirely sure if using count on the unordered_map is a good idea. It seems semantically wrong.
I think you need to revert the change in return type of RecordDelete. I would definitely check with @apavlo or @yingjunwu about why we returned different values at different conditions. And why this didn't trigger a test failure.
The is_written_ and insert_count_ can be used to make our commits/aborts faster. Because with MVCC, you'd be running such a transaction under snapshot isolation by default. Please change that and talk to Andy about probably handling that condition.

Lastly, can you please add some numbers for this branch vs the master. (Probably run TPC-C & YCSB)? So at a later stage we'd have a point of reference for our performance.

poojanilangekar · 2018-06-12T14:48:11Z

src/concurrency/timestamp_ordering_transaction_manager.cpp

@@ -655,12 +655,20 @@ ResultType TimestampOrderingTransactionManager::CommitTransaction(

  // TODO (Pooja): This might be inefficient since we will have to get the


You can remove this TODO, your code has done what the TODO was about.

poojanilangekar · 2018-06-12T14:48:53Z

src/concurrency/timestamp_ordering_transaction_manager.cpp

@@ -807,11 +815,20 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction(
  // Iterate through each item pointer in the read write set
  // TODO (Pooja): This might be inefficient since we will have to get the


Again, you can remove this TODO.

poojanilangekar · 2018-06-12T14:51:51Z

src/concurrency/transaction_context.cpp

@@ -80,102 +78,63 @@ void TransactionContext::Init(const size_t thread_id,

  isolation_level_ = isolation;

-  is_written_ = false;


I am a little unsure about removing the is_written_ and insert_count_. These variables can be used to make Commits and Aborts faster. If you figure out that the is_written_ is false, you can take an entirely different code path and treat it like a READ_ONLY transaction.

That's effectively what @lmwnshn is doing in #1402.

I think it is okay if we get rid of insert_count_ but we should definitely use is_written_. It will help with multi statement transactions that are not explicitly set to read only.

poojanilangekar · 2018-06-12T14:57:29Z

src/concurrency/transaction_context.cpp

 }

 void TransactionContext::RecordRead(const ItemPointer &location) {
-
+  PELOTON_ASSERT(rw_set_.count(location) == 0 ||


Why count? Shouldn't this be a find? It makes sense to usecount withunordered_multimap, the unordered_map can never contain more than one element with the same key.

Shouldn't really make a difference, but sure I'll change it.

poojanilangekar · 2018-06-12T15:02:49Z

src/concurrency/transaction_context.cpp

 }

-bool TransactionContext::RecordDelete(const ItemPointer &location) {
+void TransactionContext::RecordDelete(const ItemPointer &location) {


Please revert this.
We used to return true in case location contained RWType::INSERT and false otherwise. I believe we should maintain these semantics.

That value was never captured in any of the calls and struck me as cruft from other CC implementations but I can change it back.

poojanilangekar · 2018-06-12T15:04:04Z

test/common/lock_free_array_test.cpp

+  {
+    LockFreeArray<value_type> array;
+
+    value_type invalid_value = 6288;


Please use INVALID_OID instead.

mbutrovich · 2018-06-12T15:58:44Z

@poojanilangekar Regarding comment 2, there are no tests for transaction_context and for TimestampOrderingTransactionManager, well... :(

Maybe I can adapt the tests I wrote for GC fixes to have EXPECTs that test TOTM. It would be helpful to have some sort of baselines if we're making changes to TOTM's guts.

For performance, our numbers still seem way too variable to take away anything meaningful, but I'll see if a bunch of runs will smooth that out.

coveralls · 2018-06-12T19:14:11Z

Coverage increased (+0.07%) to 77.029% when pulling 21253c4 on mbutrovich:friday_night into 308a669 on cmu-db:master.

mbutrovich · 2018-06-12T19:20:03Z

Like I mentioned, hard to take away too much from oltpbench runs right now due to variability. I ran master and the friday_night branch with the attached configs on my laptop:

TPC-C (scale factor 4, 4 terminals, 60 seconds, repeated 10 times):
master: mu: 334.63, sigma: 15.48
friday_night: mu: 347.39, sigma: 40.44

YCSB (scale factor 1000, 4 terminals, read only, 60 seconds, repeated 10 times):
master: mu: 16671.82, sigma: 67.89
friday_night: mu: 16669.69, sigma: 92.82

oltpbench_configs.zip

mbutrovich · 2018-06-12T21:01:00Z

I also did some sampling with dtrace, configs from the previous comment:

TPC-C

Sampled calls to StorageManager::GetTileGroup (cuckoo hash lookups) from CommitTransaction:
master: 7317
friday_night: 6880

Sampled calls to tbb::internal_find:
master: 8032
friday_night: 7815

YCSB read-only

Sampled calls to StorageManager::GetTileGroup (cuckoo hash lookups) from CommitTransaction:
master: 2475
friday_night: 2266

Sampled calls to tbb::internal_find:
master: 6331
friday_night: 6070

poojanilangekar · 2018-06-13T16:42:26Z

@mbutrovich Do you have an idea about why the performance of this branch has higher variance?

Yes, it would be great if you could add a couple of tests, to the TimestampOrderingTransactionManager.

mbutrovich · 2018-06-13T20:13:51Z

@poojanilangekar Luck of the draw with Peloton and oltpbench, really. Also this is on my laptop where I don't have a ton of control over background tasks. I ran them again today:

TPC-C (scale factor 4, 4 terminals, 60 seconds, repeated 10 times):
master: mu: 332.59, sigma: 11.98
friday_night: mu: 333.06, sigma: 13.33

YCSB (scale factor 1000, 4 terminals, read only, 60 seconds, repeated 10 times):
master: mu: 16689.99, sigma: 127.87
friday_night: mu: 16706.10, sigma: 122.95

Again, still tough to take too much away from oltpbench right now. Regarding tests for TOTM, not sure this is the PR for it.

I'll put the is_written_ flag back.

Refactor of RecordDelete to reduce lookups.

tomasic · 2018-06-13T20:20:04Z

might help to get the abort rate reported also ...

…

On Wed, Jun 13, 2018 at 3:13 PM Matt Butrovich ***@***.***> wrote: @poojanilangekar <https://github.com/poojanilangekar> Luck of the draw with Peloton and oltpbench, really. Also this is on my laptop where I don't have a ton of control over background tasks. I ran them again today: *TPC-C (scale factor 4, 4 terminals, 60 seconds, repeated 10 times):* master: mu: 332.59, sigma: 11.98 friday_night: mu: 333.06, sigma: 13.33 *YCSB (scale factor 1000, 4 terminals, read only, 60 seconds, repeated 10 times):* master: mu: 16689.99, sigma: 127.87 friday_night: mu: 16706.10, sigma: 122.95 Again, still tough to take too much away from oltpbench right now. Regarding tests for TOTM, not sure this is the PR for it. I'll put the is_written_ flag back. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1401 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABS8HLEXnUVFldlKELXUud4RJWi6A6BWks5t8XKBgaJpZM4UjN9u> .

-- Anthony Tomasic Language Technologies Institute Carnegie Mellon University http://www.tiramisutransit.com *http://mcds.cs.cmu.edu <http://mcds.cs.cmu.edu>* http://www.cs.cmu.edu/~tomasic

poojanilangekar · 2018-06-13T20:27:16Z

LGTM. I think this should be merged in once the build passes.

mbutrovich · 2018-06-13T21:28:54Z

@tomasic I just ran dtrace to approximate abort rates (never tried Peloton's internal stats, and not sure how much they slow the system down). dtrace dropped throughput by ~10%, but:

TPC-C:
CommitTransaction() samples: 27261
AbortTransaction() samples: 1

YCSB:
CommitTransaction() samples: 17997
AbortTransaction() samples: 0

It doesn't seem like aborts are an issue, at least under these oltpbench configs on my laptop.

tomasic · 2018-06-13T21:34:10Z

Thanks - i just wondered because we are off by a factor of 100 for tpc-c and the function tracing stuff isn’t revealing obvious holes. So now I suspect locks and latches. Anthony

On Wed, Jun 13, 2018 at 4:28 PM Matt Butrovich ***@***.***> wrote: @tomasic <https://github.com/tomasic> I just ran dtrace to approximate abort rates (never tried Peloton's internal stats, and not sure how much they slow the system down). dtrace dropped throughput by ~10%, but: *TPC-C:* CommitTransaction() samples: 27261 AbortTransaction() samples: 1 *YCSB:* CommitTransaction() samples: 17997 AbortTransaction() samples: 0 It doesn't seem like aborts are an issue, at least under these oltpbench configs on my laptop. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1401 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABS8HOMB325becreskT-yyYzLfN1TOFBks5t8YQYgaJpZM4UjN9u> .

-- Anthony Tomasic Language Technologies Institute Carnegie Mellon University http://www.tiramisutransit.com *http://mcds.cs.cmu.edu <http://mcds.cs.cmu.edu>* http://www.cs.cmu.edu/~tomasic

stale

RWSet performance fixes, LockFreeArray performance fixes and tests

mbutrovich added the ready_for_review label Jun 11, 2018

mbutrovich requested review from tcm-marcel and poojanilangekar June 11, 2018 18:55

tcm-marcel reviewed Jun 12, 2018

View reviewed changes

tcm-marcel previously requested changes Jun 12, 2018

View reviewed changes

poojanilangekar suggested changes Jun 12, 2018

View reviewed changes

mbutrovich force-pushed the friday_night branch from 225ee25 to bf15969 Compare June 12, 2018 16:33

mbutrovich added 14 commits June 13, 2018 16:14

Refactor of transaction_context to reduce lookups.

a9847aa

Change FindValid to Find in DataTable.

96431ee

Try not to look up TileGroupHeader in Abort and Commit as much.

a8b1b0c

Change LockFreeArray::FindValid from O(n) to O(1).

76e0998

Refactor.

e8e5de7

Lock_free_array refactor, documentation, and added more tests.

58aa8c0

Refactor of RecordDelete to reduce lookups.

Formatting.

344c914

Fix the last_tile_group_id optimization in TOTM.

8afd1b7

Fix RecordUpdate based on PR feedback.

6779ba7

Formatting.

07d75b3

PR feedback changes.

c37e6ef

More PR feedback changes.

2aed22b

Slight logic change in RecordUpdate.

ac0a760

Fix typo.

ad0381d

mbutrovich force-pushed the friday_night branch from bf15969 to ad0381d Compare June 13, 2018 20:15

poojanilangekar previously approved these changes Jun 13, 2018

View reviewed changes

mbutrovich added 2 commits June 13, 2018 16:47

Added is_written_ flag back in for potential future optimizations.

2db074c

Formatting.

d5583f0

mbutrovich dismissed poojanilangekar’s stale review via d5583f0 June 13, 2018 20:48

tli2 added 2 commits June 14, 2018 13:48

Merge branch 'master' into friday_night

30fddc2

Merge branch 'master' into friday_night

21253c4

mbutrovich mentioned this pull request Jun 15, 2018

Read-Only TxnContext Interface; Read-Only (single-statement select, txn) optimizations #1402

Open

poojanilangekar added accepted and removed ready_for_review labels Jun 15, 2018

poojanilangekar approved these changes Jun 15, 2018

View reviewed changes

tli2 merged commit e53e5b4 into cmu-db:master Jun 15, 2018

mbutrovich deleted the friday_night branch June 15, 2018 15:49

mbutrovich mentioned this pull request Jun 19, 2018

Performance fix: replace WorkerPool sleeping with condition variable #1419

Open

mtunique pushed a commit to mtunique/peloton that referenced this pull request Apr 16, 2019

Merge pull request cmu-db#1401 from mbutrovich/friday_night

cc3346b

RWSet performance fixes, LockFreeArray performance fixes and tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

mbutrovich commented Jun 11, 2018 •

edited

Loading

tcm-marcel Jun 12, 2018

mbutrovich Jun 12, 2018 •

edited

Loading

tcm-marcel left a comment

tcm-marcel Jun 12, 2018

mbutrovich Jun 12, 2018

tcm-marcel Jun 12, 2018

mbutrovich Jun 12, 2018 •

edited

Loading

tcm-marcel Jun 12, 2018

mbutrovich Jun 12, 2018

poojanilangekar Jun 12, 2018

mbutrovich commented Jun 12, 2018

poojanilangekar left a comment

poojanilangekar Jun 12, 2018

poojanilangekar Jun 12, 2018

poojanilangekar Jun 12, 2018

mbutrovich Jun 12, 2018

poojanilangekar Jun 13, 2018

poojanilangekar Jun 12, 2018

mbutrovich Jun 12, 2018

poojanilangekar Jun 12, 2018

mbutrovich Jun 12, 2018 •

edited

Loading

poojanilangekar Jun 12, 2018

mbutrovich commented Jun 12, 2018 •

edited

Loading

coveralls commented Jun 12, 2018 •

edited

Loading

mbutrovich commented Jun 12, 2018

mbutrovich commented Jun 12, 2018

poojanilangekar commented Jun 13, 2018

mbutrovich commented Jun 13, 2018

tomasic commented Jun 13, 2018 via email

poojanilangekar commented Jun 13, 2018

mbutrovich commented Jun 13, 2018

tomasic commented Jun 13, 2018 via email

		@@ -655,12 +655,20 @@ ResultType TimestampOrderingTransactionManager::CommitTransaction(

		// TODO (Pooja): This might be inefficient since we will have to get the

		@@ -807,11 +815,20 @@ ResultType TimestampOrderingTransactionManager::AbortTransaction(
		// Iterate through each item pointer in the read write set
		// TODO (Pooja): This might be inefficient since we will have to get the

		@@ -80,102 +78,63 @@ void TransactionContext::Init(const size_t thread_id,

		isolation_level_ = isolation;

		is_written_ = false;

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

RWSet performance fixes, LockFreeArray performance fixes and tests #1401

Conversation

mbutrovich commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

mbutrovich Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

tcm-marcel 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

mbutrovich Jun 12, 2018 • edited Loading

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

mbutrovich commented Jun 12, 2018

poojanilangekar 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

mbutrovich Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbutrovich commented Jun 12, 2018 • edited Loading

coveralls commented Jun 12, 2018 • edited Loading

mbutrovich commented Jun 12, 2018

mbutrovich commented Jun 12, 2018

poojanilangekar commented Jun 13, 2018

mbutrovich commented Jun 13, 2018

tomasic commented Jun 13, 2018 via email

poojanilangekar commented Jun 13, 2018

mbutrovich commented Jun 13, 2018

tomasic commented Jun 13, 2018 via email

mbutrovich commented Jun 11, 2018 •

edited

Loading

mbutrovich Jun 12, 2018 •

edited

Loading

mbutrovich Jun 12, 2018 •

edited

Loading

mbutrovich Jun 12, 2018 •

edited

Loading

mbutrovich commented Jun 12, 2018 •

edited

Loading

coveralls commented Jun 12, 2018 •

edited

Loading