Merge lp:~vkolesnikov/pbxt/pbxt-mmap-direct-read-deadlock into lp:pbxt

[Vladimir Kolesnikov]

[lp:~vkolesnikov/pbxt/pbxt-mmap-direct-read-deadlock]

565. By Vladimir Kolesnikov on 2009-03-04

changed amotic operations code to use XADD

566. By Vladimir Kolesnikov on 2009-03-04

minor fixes to locking

567. By Vladimir Kolesnikov on 2009-03-04

removed wrong assertion

[Paul McCullagh (paul-mccullagh)]

Hi Vlad,

This implementation can be optimized a bit.

In particular, we should not always lock and copy the record header
again after every call to tab_visible().

This is only required in the case that we continue to load the record
after the switch (tab_vis) statement.

This saves time in all cases where the tab_visible() call does not
return a valid record.

On Mar 3, 2009, at 8:10 PM, Vladimir Kolesnikov wrote:

> Vladimir Kolesnikov has proposed merging lp:~vkolesnikov/pbxt/pbxt-
> mmap-direct-read-deadlock into lp:pbxt.
>
> Requested reviews:
> PBXT Core (pbxt-core)
> --
> https://code.launchpad.net/~vkolesnikov/pbxt/pbxt-mmap-direct-read-
> deadlock/+merge/4112
> Your team PBXT Core is subscribed to branch lp:pbxt.
> === modified file 'src/table_xt.cc'
> --- src/table_xt.cc 2009-02-27 09:22:44 +0000
> +++ src/table_xt.cc 2009-03-03 19:02:32 +0000
> @@ -4868,6 +4868,8 @@
> xtRecordID new_rec_id;
> xtBool ptr_locked;
> xtRecordID invalid_rec = 0;
> + XTTabRecHeadDRec rec_head;
> + int tab_vis;
>
> next_page:
> if (!ot->ot_on_page) {
> @@ -4900,6 +4902,9 @@
> XT_LOCK_MEMORY_PTR(buff_ptr, ot->ot_rec_file,
> xt_rec_id_to_rec_offset(tab, ot->ot_seq_rec_id), tab-
> >tab_rows.tci_page_size, &ot->ot_thread->st_statistics.st_rec, ot-
> >ot_thread);
> if (!buff_ptr)
> return FAILED;
> + memcpy(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec));
> + XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> + buff_ptr = (xtWord1 *)&rec_head;
> }
>
> /* This is the current record: */
> @@ -4911,20 +4916,29 @@
> ot->ot_seq_offset += rec_size;
>
> retry:
> - switch (tab_visible(ot, (XTTabRecHeadDPtr) buff_ptr, &new_rec_id)) {
> + tab_vis = tab_visible(ot, (XTTabRecHeadDPtr) buff_ptr, &new_rec_id);
> + if (ptr_locked) {
> + XT_LOCK_MEMORY_PTR(buff_ptr, ot->ot_rec_file,
> xt_rec_id_to_rec_offset(tab, ot->ot_seq_rec_id - 1),
> + tab->tab_rows.tci_page_size, &ot->ot_thread-
> >st_statistics.st_rec, ot->ot_thread);
> +
> + if (!buff_ptr)
> + return FAILED;
> +
> + if (memcmp(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec))) {
> + memcpy(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec));
> + XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> + goto retry;
> + }
> +
> + XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> + }
> +
> + switch (tab_vis) {
> case FALSE:
> - if (ptr_locked) {
> - XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> - ptr_locked = FALSE;
> - }
> goto next_record;
> case XT_ERR:
> goto failed;
> case XT_NEW:
> - if (ptr_locked) {
> - XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> - ptr_locked = FALSE;
> - }
> buff_ptr = ot->ot_row_rbuffer;
> if (!xt_tab_get_rec_data(ot, new_rec_id, rec_size, ot-
> >ot_row_rbuffer))
> return XT_ERR;
> @@ -4937,12 +4951,6 @@
> /* Don't re-read for the same record twice: */
> invalid_rec = ot->ot_curr_rec_id;
>
> - /* Unlock the memory map: */
> - if (ptr_locked) {
> - XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
> - ptr_locked = FALSE;
> - }
> -
> /* Undo move to next: */
> ot->ot_seq_rec_id--;
> ot->ot_seq_offset -= rec_size;
> @@ -49...

[lp:~vkolesnikov/pbxt/pbxt-mmap-direct-read-deadlock]

568. By Vladimir Kolesnikov on 2009-03-05

removed unnecessary checking of record header change after tab_visible

569. By Vladimir Kolesnikov on 2009-03-05

fixed a bug in the new locking code - when searching for a row lock, lock ranges were not taken into account

570. By Vladimir Kolesnikov on 2009-03-05

fixed asm atomic operations code for windows

571. By Vladimir Kolesnikov on 2009-03-05

merge from upstream

572. By Vladimir Kolesnikov on 2009-03-05

changed linux version of atomic operations to use lock/add instead of xadd

573. By Paul McCullagh on 2009-03-05

Different logic in the item not found case

574. By Vladimir Kolesnikov on 2009-03-05

another fix to row-locking

=== modified file 'src/table_xt.cc'
--- src/table_xt.cc	2009-02-27 09:22:44 +0000
+++ src/table_xt.cc	2009-03-03 19:02:32 +0000
@@ -4868,6 +4868,8 @@
 	xtRecordID				new_rec_id;
 	xtBool					ptr_locked;
 	xtRecordID				invalid_rec = 0;
+	XTTabRecHeadDRec		rec_head;
+	int						tab_vis;
 
 	next_page:
 	if (!ot->ot_on_page) {
@@ -4900,6 +4902,9 @@
 		XT_LOCK_MEMORY_PTR(buff_ptr, ot->ot_rec_file, xt_rec_id_to_rec_offset(tab, ot->ot_seq_rec_id), tab->tab_rows.tci_page_size, &ot->ot_thread->st_statistics.st_rec, ot->ot_thread);
 		if (!buff_ptr)
 			return FAILED;
+		memcpy(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec));
+		XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
+		buff_ptr = (xtWord1 *)&rec_head;
 	}
 
 	/* This is the current record: */
@@ -4911,20 +4916,29 @@
 	ot->ot_seq_offset += rec_size;
 
 	retry:
-	switch (tab_visible(ot, (XTTabRecHeadDPtr) buff_ptr, &new_rec_id)) {
+	tab_vis = tab_visible(ot, (XTTabRecHeadDPtr) buff_ptr, &new_rec_id);
+	if (ptr_locked) {
+		XT_LOCK_MEMORY_PTR(buff_ptr, ot->ot_rec_file, xt_rec_id_to_rec_offset(tab, ot->ot_seq_rec_id - 1), 
+			tab->tab_rows.tci_page_size, &ot->ot_thread->st_statistics.st_rec, ot->ot_thread);
+
+		if (!buff_ptr)
+			return FAILED;
+
+		if (memcmp(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec))) {
+			memcpy(&rec_head, buff_ptr, sizeof(XTTabRecHeadDRec));
+			XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
+			goto retry;
+		}
+
+		XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
+	}
+
+	switch (tab_vis) {
 		case FALSE:
-			if (ptr_locked) {
-				XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
-				ptr_locked = FALSE;
-			}
 			goto next_record;
 		case XT_ERR:
 			goto failed;
 		case XT_NEW:
-			if (ptr_locked) {
-				XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
-				ptr_locked = FALSE;
-			}
 			buff_ptr = ot->ot_row_rbuffer;
 			if (!xt_tab_get_rec_data(ot, new_rec_id, rec_size, ot->ot_row_rbuffer))
 				return XT_ERR;
@@ -4937,12 +4951,6 @@
 				/* Don't re-read for the same record twice: */
 				invalid_rec = ot->ot_curr_rec_id;
 
-				/* Unlock the memory map: */
-				if (ptr_locked) {
-					XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
-					ptr_locked = FALSE;
-				}
-
 				/* Undo move to next: */
 				ot->ot_seq_rec_id--;
 				ot->ot_seq_offset -= rec_size;
@@ -4992,15 +5000,11 @@
 			break;
 		}
 	}
-	if (ptr_locked)
-		XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
 
 	*eof = FALSE;
 	return OK;
 
 	failed:
-	if (ptr_locked)
-		XT_UNLOCK_MEMORY_PTR(ot->ot_rec_file, ot->ot_thread);
 	return FAILED;
 }