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;
> @@ -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;
> }
>
>



--
Paul McCullagh
PrimeBase Technologies
www.primebase.org
www.blobstreaming.org
pbxt.blogspot.com