Code review comment for lp:~vkolesnikov/pbxt/pbxt-mmap-direct-read-deadlock

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