From 86cc5fe9c873a1578f95ed427130312a35b241fc Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Fri, 29 Sep 2017 12:35:59 +0930 Subject: [PATCH 1/5] Remove unused imports --- extensions/sticky/js/forum/dist/extension.js | 9 +++------ extensions/sticky/js/forum/src/main.js | 3 +-- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/extensions/sticky/js/forum/dist/extension.js b/extensions/sticky/js/forum/dist/extension.js index d68918337..0c6695427 100644 --- a/extensions/sticky/js/forum/dist/extension.js +++ b/extensions/sticky/js/forum/dist/extension.js @@ -156,15 +156,12 @@ System.register('flarum/sticky/components/DiscussionStickiedPost', ['flarum/comp });; 'use strict'; -System.register('flarum/sticky/main', ['flarum/extend', 'flarum/app', 'flarum/Model', 'flarum/models/Discussion', 'flarum/sticky/components/DiscussionStickiedPost', 'flarum/sticky/addStickyBadge', 'flarum/sticky/addStickyControl', 'flarum/sticky/addStickyExcerpt'], function (_export, _context) { +System.register('flarum/sticky/main', ['flarum/app', 'flarum/Model', 'flarum/models/Discussion', 'flarum/sticky/components/DiscussionStickiedPost', 'flarum/sticky/addStickyBadge', 'flarum/sticky/addStickyControl', 'flarum/sticky/addStickyExcerpt'], function (_export, _context) { "use strict"; - var extend, notificationType, app, Model, Discussion, DiscussionStickiedPost, addStickyBadge, addStickyControl, addStickyExcerpt; + var app, Model, Discussion, DiscussionStickiedPost, addStickyBadge, addStickyControl, addStickyExcerpt; return { - setters: [function (_flarumExtend) { - extend = _flarumExtend.extend; - notificationType = _flarumExtend.notificationType; - }, function (_flarumApp) { + setters: [function (_flarumApp) { app = _flarumApp.default; }, function (_flarumModel) { Model = _flarumModel.default; diff --git a/extensions/sticky/js/forum/src/main.js b/extensions/sticky/js/forum/src/main.js index 1cfe374e2..838e705f3 100644 --- a/extensions/sticky/js/forum/src/main.js +++ b/extensions/sticky/js/forum/src/main.js @@ -1,4 +1,3 @@ -import { extend, notificationType } from 'flarum/extend'; import app from 'flarum/app'; import Model from 'flarum/Model'; import Discussion from 'flarum/models/Discussion'; @@ -17,4 +16,4 @@ app.initializers.add('flarum-sticky', () => { addStickyBadge(); addStickyControl(); addStickyExcerpt(); -}); \ No newline at end of file +}); From 7ff57e1ba448636dc08b5dcd18ce39c68f941f48 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Wed, 8 Nov 2017 11:34:37 +1030 Subject: [PATCH 2/5] Refactor sticky order clause into a subquery Based on some limited testing, using a subquery seems to outperform a join in this case (the join was invoking a temporary table, which is always a bad sign). This also adds logic to fix a bug where sticky discussions would remain at the top even when marked as read using the "mark all as read" button. I thought we had an open issue for this somewhere, but I can't seem to find one. ref #988 #1003 --- .../Listener/PinStickiedDiscussionsToTop.php | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php index 74352c79e..4c284c36b 100755 --- a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php +++ b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php @@ -45,10 +45,17 @@ class PinStickiedDiscussionsToTop $search = $event->search; $query = $search->getQuery(); + // TODO: ideally we might like to consider an event in core that is + // fired before the sort criteria is applied to the query (ie. + // immediately after gambits are applied) so that we can add the + // following order logic to the start without using array_unshift. + if (! is_array($query->orders)) { $query->orders = []; } + // If we are viewing a specific tag, then pin all stickied + // discussions to the top no matter what. foreach ($search->getActiveGambits() as $gambit) { if ($gambit instanceof TagGambit) { array_unshift($query->orders, ['column' => 'is_sticky', 'direction' => 'desc']); @@ -57,21 +64,28 @@ class PinStickiedDiscussionsToTop } } - $query->leftJoin('users_discussions', function ($join) use ($search) { - $join->on('users_discussions.discussion_id', '=', 'discussions.id') - ->where('discussions.is_sticky', '=', true) - ->where('users_discussions.user_id', '=', $search->getActor()->id); - }); - - // TODO: Might be quicker to do a subquery in the order clause than a join? - $grammar = $query->getGrammar(); - $readNumber = $grammar->wrap('users_discussions.read_number'); - $lastPostNumber = $grammar->wrap('discussions.last_post_number'); + // Otherwise, if we are viewing "all discussions" or similar, only + // pin stickied discussions to the top if they are unread. To do + // this we construct an order clause containing a subquery which + // determines whether or not the discussion is unread. + $subquery = $query->newQuery() + ->selectRaw(1) + ->from('users_discussions as sticky') + ->whereRaw('sticky.discussion_id = discussions.id') + ->where('sticky.user_id', '=', $search->getActor()->id) + ->where(function ($query) { + $query->whereNull('sticky.read_number') + ->orWhereRaw('discussions.last_post_number > sticky.read_number'); + }) + ->where('discussions.last_time', '>', $search->getActor()->read_time ?: 0); array_unshift($query->orders, [ 'type' => 'raw', - 'sql' => "(is_sticky AND ($readNumber IS NULL OR $lastPostNumber > $readNumber)) desc" + 'sql' => "(is_sticky and exists ({$subquery->toSql()})) desc" ]); + + $orderBindings = $query->getRawBindings()['order']; + $query->setBindings(array_merge($subquery->getBindings(), $orderBindings), 'order'); } } } From 0ce00c6d169159c5ba65f1fec8b4ebb6217b2bba Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Sun, 12 Nov 2017 21:28:17 +1030 Subject: [PATCH 3/5] Performance: Refactor SQL that pins unread sticky posts to the top Ordering by `is_sticky and (unread subquery) desc` removes MySQL's ability to use an index for ordering by `last_time`, which triggers a filesort across the whole discussions table which is BAD. This commit uses a union to add all stickied discussions to the query. The results of the unioned queries are then ordered by the `is_sticky and (unread subquery)` criteria, so the filesort only takes place on a maximum of limit * 2 rows. Big performance win when you get up to thousands of discussions! --- .../Listener/PinStickiedDiscussionsToTop.php | 58 ++++++++++--------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php index 4c284c36b..1f0f54e8d 100755 --- a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php +++ b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php @@ -25,7 +25,7 @@ class PinStickiedDiscussionsToTop public function subscribe(Dispatcher $events) { $events->listen(ConfigureDiscussionGambits::class, [$this, 'addStickyGambit']); - $events->listen(ConfigureDiscussionSearch::class, [$this, 'reorderSearch']); + $events->listen(ConfigureDiscussionSearch::class, [$this, 'reorderSearch'], -100); } /** @@ -45,18 +45,15 @@ class PinStickiedDiscussionsToTop $search = $event->search; $query = $search->getQuery(); - // TODO: ideally we might like to consider an event in core that is - // fired before the sort criteria is applied to the query (ie. - // immediately after gambits are applied) so that we can add the - // following order logic to the start without using array_unshift. - if (! is_array($query->orders)) { $query->orders = []; } // If we are viewing a specific tag, then pin all stickied // discussions to the top no matter what. - foreach ($search->getActiveGambits() as $gambit) { + $gambits = $search->getActiveGambits(); + + foreach ($gambits as $gambit) { if ($gambit instanceof TagGambit) { array_unshift($query->orders, ['column' => 'is_sticky', 'direction' => 'desc']); @@ -64,28 +61,35 @@ class PinStickiedDiscussionsToTop } } - // Otherwise, if we are viewing "all discussions" or similar, only - // pin stickied discussions to the top if they are unread. To do - // this we construct an order clause containing a subquery which - // determines whether or not the discussion is unread. - $subquery = $query->newQuery() - ->selectRaw(1) - ->from('users_discussions as sticky') - ->whereRaw('sticky.discussion_id = discussions.id') - ->where('sticky.user_id', '=', $search->getActor()->id) - ->where(function ($query) { - $query->whereNull('sticky.read_number') - ->orWhereRaw('discussions.last_post_number > sticky.read_number'); - }) - ->where('discussions.last_time', '>', $search->getActor()->read_time ?: 0); + // Otherwise, if we are viewing "all discussions", only pin stickied + // discussions to the top if they are unread. To do this in a + // performant way we create another query which will select all + // stickied discussions, marry them into the main query, and then + // reorder the unread ones up to the top. + if (empty($gambits)) { + $sticky = clone $query; + $sticky->where('is_sticky', true); + $sticky->limit = $sticky->offset = $sticky->orders = null; - array_unshift($query->orders, [ - 'type' => 'raw', - 'sql' => "(is_sticky and exists ({$subquery->toSql()})) desc" - ]); + $query->union($sticky); - $orderBindings = $query->getRawBindings()['order']; - $query->setBindings(array_merge($subquery->getBindings(), $orderBindings), 'order'); + $read = $query->newQuery() + ->selectRaw(1) + ->from('users_discussions as sticky') + ->whereRaw('sticky.discussion_id = id') + ->where('sticky.user_id', '=', $search->getActor()->id) + ->whereRaw('sticky.read_number >= last_post_number'); + + // Add the bindings manually (rather than as the second + // argument in orderByRaw) for now due to a bug in Laravel which + // would add the bindings in the wrong order. + $query->orderByRaw('is_sticky and not exists ('.$read->toSql().') and last_time > ? desc') + ->addBinding(array_merge($read->getBindings(), [$search->getActor()->read_time ?: 0]), 'union'); + + $query->unionOrders = array_merge($query->unionOrders, $query->orders); + $query->unionLimit = $query->limit; + $query->unionOffset = $query->offset; + } } } } From c7d7b2d36003b3c694806af67f9a8bf84e6b2ced Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Mon, 13 Nov 2017 09:17:55 +1030 Subject: [PATCH 4/5] Fix subsequent pages of results not working properly --- .../sticky/src/Listener/PinStickiedDiscussionsToTop.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php index 1f0f54e8d..78fc69610 100755 --- a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php +++ b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php @@ -69,7 +69,7 @@ class PinStickiedDiscussionsToTop if (empty($gambits)) { $sticky = clone $query; $sticky->where('is_sticky', true); - $sticky->limit = $sticky->offset = $sticky->orders = null; + $sticky->orders = null; $query->union($sticky); @@ -89,6 +89,9 @@ class PinStickiedDiscussionsToTop $query->unionOrders = array_merge($query->unionOrders, $query->orders); $query->unionLimit = $query->limit; $query->unionOffset = $query->offset; + + $query->limit = $sticky->limit = $query->offset + $query->limit; + $query->offset = $sticky->offset = null; } } } From 20f6a256d5ad8c434bb1bbd17c5fa19f072d5708 Mon Sep 17 00:00:00 2001 From: Toby Zerner Date: Mon, 13 Nov 2017 13:18:36 +1030 Subject: [PATCH 5/5] Only touch orders property if we're going to add to it --- .../sticky/src/Listener/PinStickiedDiscussionsToTop.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php index 78fc69610..af454efdf 100755 --- a/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php +++ b/extensions/sticky/src/Listener/PinStickiedDiscussionsToTop.php @@ -45,16 +45,16 @@ class PinStickiedDiscussionsToTop $search = $event->search; $query = $search->getQuery(); - if (! is_array($query->orders)) { - $query->orders = []; - } - // If we are viewing a specific tag, then pin all stickied // discussions to the top no matter what. $gambits = $search->getActiveGambits(); foreach ($gambits as $gambit) { if ($gambit instanceof TagGambit) { + if (! is_array($query->orders)) { + $query->orders = []; + } + array_unshift($query->orders, ['column' => 'is_sticky', 'direction' => 'desc']); return;