From bb16898b1c73073e0442de72f2af133a3bd39713 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Thu, 4 Feb 2010 14:50:20 -0800 Subject: Inbox::streamNotices() with deletion compensation: inbox paging should more or less work with deleted items now. No change in efficiency for the common case where nothing's deleted: does the same bulk fetch of just the notices we think we'll need as before, then if we turned up short keeps checking one by one until we've filled up to our $limit. This can leave us with overlap between pages, but we already have that when new messages come in between clicks; seems to be the lesser of evils versus not getting a 'before' button. More permanent fix for that will be to switch timeline paging in the UI to use notice IDs. --- classes/Inbox.php | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 63 insertions(+), 3 deletions(-) (limited to 'classes/Inbox.php') diff --git a/classes/Inbox.php b/classes/Inbox.php index 26b27d2b5..029410799 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -32,6 +32,7 @@ require_once INSTALLDIR.'/classes/Memcached_DataObject.php'; class Inbox extends Memcached_DataObject { const BOXCAR = 128; + const MAX_NOTICES = 1024; ###START_AUTOCODE /* the code below is auto generated do not remove the above tag */ @@ -81,7 +82,7 @@ class Inbox extends Memcached_DataObject $ni->selectAdd(); $ni->selectAdd('notice_id'); $ni->orderBy('notice_id DESC'); - $ni->limit(0, 1024); + $ni->limit(0, self::MAX_NOTICES); if ($ni->find()) { while($ni->fetch()) { @@ -115,9 +116,11 @@ class Inbox extends Memcached_DataObject $result = $inbox->query(sprintf('UPDATE inbox '. 'set notice_ids = concat(cast(0x%08x as binary(4)), '. - 'substr(notice_ids, 1, 4092)) '. + 'substr(notice_ids, 1, %d)) '. 'WHERE user_id = %d', - $notice_id, $user_id)); + $notice_id, + 4 * (self::MAX_NOTICES - 1), + $user_id)); if ($result) { self::blow('inbox:user_id:%d', $user_id); @@ -173,4 +176,61 @@ class Inbox extends Memcached_DataObject return $ids; } + + /** + * Wrapper for Inbox::stream() and Notice::getStreamByIds() returning + * additional items up to the limit if we were short due to deleted + * notices still being listed in the inbox. + * + * The fast path (when no items are deleted) should be just as fast; the + * offset parameter is applied *before* lookups for maximum efficiency. + * + * This means offset-based paging may show duplicates, but similar behavior + * already exists when new notices are posted between page views, so we + * think people will be ok with this until id-based paging is introduced + * to the user interface. + * + * @param int $user_id + * @param int $offset skip past the most recent N notices (after since_id checks) + * @param int $limit + * @param mixed $since_id return only notices after but not including this id + * @param mixed $max_id return only notices up to and including this id + * @param mixed $since obsolete/ignored + * @param mixed $own ignored? + * @return array of Notice objects + * + * @todo consider repacking the inbox when this happens? + */ + function streamNotices($user_id, $offset, $limit, $since_id, $max_id, $since, $own=false) + { + $ids = self::stream($user_id, $offset, self::MAX_NOTICES, $since_id, $max_id, $since, $own); + + // Do a bulk lookup for the first $limit items + // Fast path when nothing's deleted. + $firstChunk = array_slice($ids, 0, $limit); + $notices = Notice::getStreamByIds($firstChunk); + + $wanted = count($firstChunk); // raw entry count in the inbox up to our $limit + if ($notices->N >= $wanted) { + common_log(LOG_DEBUG, __METHOD__ . " ok got $wanted items"); + return $notices; + } + + common_log(LOG_DEBUG, __METHOD__ . " got $notices->N of $wanted items, getting more"); + // There were deleted notices, we'll need to look for more. + assert($notices instanceof ArrayWrapper); + $items = $notices->_items; + $remainder = array_slice($ids, $limit); + + while (count($items) < $wanted && count($remainder) > 0) { + $notice = Notice::staticGet(array_shift($remainder)); + if ($notice) { + common_log(LOG_DEBUG, __METHOD__ . " got another one"); + $items[] = $notice; + } else { + common_log(LOG_DEBUG, __METHOD__ . " skipping another deleted one"); + } + } + return new ArrayWrapper($items); + } } -- cgit v1.2.3-54-g00ecf From 4502bea9a86fe5992eb9b359d90f0c1f004998c1 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Thu, 4 Feb 2010 15:16:27 -0800 Subject: drop debug messages from inbox deletion fix --- classes/Inbox.php | 4 ---- 1 file changed, 4 deletions(-) (limited to 'classes/Inbox.php') diff --git a/classes/Inbox.php b/classes/Inbox.php index 029410799..be62611a1 100644 --- a/classes/Inbox.php +++ b/classes/Inbox.php @@ -212,11 +212,9 @@ class Inbox extends Memcached_DataObject $wanted = count($firstChunk); // raw entry count in the inbox up to our $limit if ($notices->N >= $wanted) { - common_log(LOG_DEBUG, __METHOD__ . " ok got $wanted items"); return $notices; } - common_log(LOG_DEBUG, __METHOD__ . " got $notices->N of $wanted items, getting more"); // There were deleted notices, we'll need to look for more. assert($notices instanceof ArrayWrapper); $items = $notices->_items; @@ -225,10 +223,8 @@ class Inbox extends Memcached_DataObject while (count($items) < $wanted && count($remainder) > 0) { $notice = Notice::staticGet(array_shift($remainder)); if ($notice) { - common_log(LOG_DEBUG, __METHOD__ . " got another one"); $items[] = $notice; } else { - common_log(LOG_DEBUG, __METHOD__ . " skipping another deleted one"); } } return new ArrayWrapper($items); -- cgit v1.2.3-54-g00ecf