Skip to content

Commit 9c79816

Browse files
authored
Merge pull request #969 from ASKBOT/tag-search-optimization
Tag search optimization
2 parents 285eb58 + e93eb73 commit 9c79816

4 files changed

Lines changed: 198 additions & 5 deletions

File tree

askbot/doc/source/changelog.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ Changes in Askbot
33

44
Development (not yet released)
55
------------------------------
6+
* Optimized multi-tag question search: replaced per-tag JOINs with a single subquery.
67
* Added per subnet rate-limiting feature that allows
78
limiting three types of requests: get requests,
89
post requests from watched users

askbot/models/question.py

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from copy import copy
77
from django.conf import settings as django_settings
88
from django.db import models
9-
from django.db.models import F, Q
9+
from django.db.models import Count, F, Q
1010
from django.contrib.auth.models import User
1111
from django.contrib.contenttypes.models import ContentType
1212
from django.core import cache # import cache, not from cache import cache, to be able to monkey-patch cache.cache in test cases
@@ -358,10 +358,29 @@ def run_advanced_search(self, request_user, search_state):
358358
else:
359359
meta_data['non_existing_tags'] = list()
360360

361-
# construct filter for the tag search
362-
for tag in tags:
363-
# Tags or AND-ed here, not OR-ed (i.e. we fetch only threads with all tags)
364-
qs = qs.filter(tags__name=tag)
361+
# AND across tags: fetch only threads tagged with every requested tag.
362+
# The natural ORM expression — one .filter(tags__name=tag) per tag —
363+
# produces a JOIN through the M2M for every tag, which scales
364+
# superlinearly and degrades to multi-second response times for
365+
# 6-8 tag queries even on small datasets. The subquery + HAVING
366+
# COUNT form is the relational idiom for set intersection.
367+
# Deduplicate: unified_tags() concatenates query_tags + tags,
368+
# so the same tag can appear twice; an inflated len() would
369+
# never match Count(distinct=True). Empty list means all
370+
# requested tags were unknown, in which case the search is
371+
# unfiltered by tags (the names are reported via meta_data).
372+
tags = list(set(tags))
373+
if tags:
374+
ThreadTagModel = self.model.tags.through
375+
matching_thread_ids = (
376+
ThreadTagModel.objects
377+
.filter(tag__name__in=tags)
378+
.values('thread_id')
379+
.annotate(matched=Count('tag_id', distinct=True))
380+
.filter(matched=len(tags))
381+
.values_list('thread_id', flat=True)
382+
)
383+
qs = qs.filter(id__in=matching_thread_ids)
365384
else:
366385
meta_data['non_existing_tags'] = list()
367386

askbot/tests/test_post_model.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -239,10 +239,49 @@ def test_run_adv_search_ANDing_tags(self):
239239
qs, meta_data = Thread.objects.run_advanced_search(request_user=self.user, search_state=ss.add_tag('tag1').add_tag('tag3').add_tag('tag6'))
240240
self.assertEqual(1, qs.count())
241241

242+
# 4+ tag AND: exercises the subquery+HAVING COUNT path. q4 is the
243+
# only thread tagged with all six tags so any 4-6 tag subset matches
244+
# only q4.
245+
qs, meta_data = Thread.objects.run_advanced_search(request_user=self.user, search_state=ss.add_tag('tag1').add_tag('tag2').add_tag('tag3').add_tag('tag6'))
246+
self.assertEqual(1, qs.count())
247+
self.assertEqual(self.q4.thread_id, qs[0].id)
248+
249+
qs, meta_data = Thread.objects.run_advanced_search(request_user=self.user, search_state=ss.add_tag('tag1').add_tag('tag2').add_tag('tag3').add_tag('tag4').add_tag('tag5').add_tag('tag6'))
250+
self.assertEqual(1, qs.count())
251+
self.assertEqual(self.q4.thread_id, qs[0].id)
252+
242253
ss = SearchState(scope=None, sort=None, query="#tag3", tags='tag1, tag6', author=None, page=None, user_logged_in=None)
243254
qs, meta_data = Thread.objects.run_advanced_search(request_user=self.user, search_state=ss)
244255
self.assertEqual(1, qs.count())
245256

257+
@with_settings(TAG_SEARCH_INPUT_ENABLED=False)
258+
def test_run_adv_search_ANDing_tags_duplicate_across_query_and_selector(self):
259+
# unified_tags() concatenates query_tags + tags, so the same tag in
260+
# both the #tag query syntax and the selector produces a duplicate.
261+
# The result set must still be the AND over the *distinct* tags.
262+
ss = SearchState(scope=None, sort=None, query="#tag3",
263+
tags='tag1, tag3', author=None, page=None,
264+
user_logged_in=None)
265+
qs, meta_data = Thread.objects.run_advanced_search(
266+
request_user=self.user, search_state=ss)
267+
# q1 (tag1,tag2,tag3) and q4 (tag1..tag6) both carry tag1 AND tag3
268+
self.assertEqual(2, qs.count())
269+
self.assertEqual({self.q1.thread_id, self.q4.thread_id},
270+
{t.id for t in qs})
271+
272+
@with_settings(TAG_SEARCH_INPUT_ENABLED=True)
273+
def test_run_adv_search_ANDing_tags_all_nonexistent(self):
274+
# When every requested tag is unknown, the search should not narrow
275+
# the result set; it should return all visible threads and report
276+
# the missing tags via meta_data.
277+
ss = SearchState.get_empty()
278+
ss = ss.add_tag('nope-one').add_tag('nope-two')
279+
qs, meta_data = Thread.objects.run_advanced_search(
280+
request_user=self.user, search_state=ss)
281+
self.assertEqual(4, qs.count())
282+
self.assertEqual({'nope-one', 'nope-two'},
283+
set(meta_data['non_existing_tags']))
284+
246285
def test_run_adv_search_query_author(self):
247286
ss = SearchState(scope=None, sort=None, query="@user", tags=None, author=None, page=None, user_logged_in=None)
248287
qs, meta_data = Thread.objects.run_advanced_search(request_user=self.user, search_state=ss)

scripts/perf_tag_search.py

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
"""One-off perf script for the tag-intersection query.
2+
3+
Picks a real thread that carries the largest tag set in the database, then
4+
issues GETs to /questions/tags:t1,t2,.../ for increasing tag counts so the
5+
intersection always returns at least one question. Times each request with
6+
warm-up + repeats so the numbers are stable enough to compare branches.
7+
8+
Usage:
9+
cd askbot_site
10+
../env-md/bin/python ../perf_tag_search.py --base-url http://127.0.0.1:8000
11+
12+
The dev server must already be running (manage.py runserver, gunicorn, etc.)
13+
on the URL passed in. Run the script once before applying the optimization
14+
and once after; compare the columns.
15+
"""
16+
import argparse
17+
import os
18+
import statistics
19+
import sys
20+
import time
21+
import urllib.error
22+
import urllib.parse
23+
import urllib.request
24+
25+
import django
26+
27+
28+
def setup_django():
29+
os.environ.setdefault('DJANGO_SETTINGS_MODULE', 'askbot_site.settings')
30+
if os.getcwd() not in sys.path:
31+
sys.path.insert(0, os.getcwd())
32+
django.setup()
33+
34+
35+
def pick_tag_set(min_count, max_count):
36+
"""Return a list of tag names that all co-occur on the same thread.
37+
38+
Uses the thread carrying the most tags so the intersection is guaranteed
39+
non-empty for any prefix of the returned list.
40+
"""
41+
from askbot.models import Thread
42+
from django.db.models import Count
43+
44+
threads = (
45+
Thread.objects
46+
.annotate(n_tags=Count('tags'))
47+
.filter(n_tags__gte=min_count)
48+
.order_by('-n_tags')
49+
)
50+
thread = threads.first()
51+
if thread is None:
52+
raise SystemExit(
53+
f'No thread has at least {min_count} tags - lower --min-tags or '
54+
'load a larger dataset.'
55+
)
56+
names = list(thread.tags.values_list('name', flat=True))
57+
if max_count:
58+
names = names[:max_count]
59+
print(f'Using thread id={thread.id} with {len(names)} tags: {names}')
60+
return names
61+
62+
63+
def build_url(base_url, tags):
64+
encoded = urllib.parse.quote(','.join(tags), safe=',+.-_')
65+
return f'{base_url.rstrip("/")}/questions/tags:{encoded}/'
66+
67+
68+
def time_request(url, timeout):
69+
start = time.perf_counter()
70+
with urllib.request.urlopen(url, timeout=timeout) as resp:
71+
body = resp.read()
72+
status = resp.status
73+
elapsed = time.perf_counter() - start
74+
return elapsed, status, len(body)
75+
76+
77+
def run(base_url, tags, repeats, warmups, timeout, sleep):
78+
header = f'{"N tags":>6} | {"min ms":>8} | {"med ms":>8} | {"max ms":>8} | {"bytes":>8} | status'
79+
print(header)
80+
print('-' * len(header))
81+
for n in range(1, len(tags) + 1):
82+
url = build_url(base_url, tags[:n])
83+
last_status = None
84+
last_size = None
85+
for _ in range(warmups):
86+
_, last_status, last_size = time_request(url, timeout)
87+
if sleep:
88+
time.sleep(sleep)
89+
samples = []
90+
for _ in range(repeats):
91+
elapsed, last_status, last_size = time_request(url, timeout)
92+
samples.append(elapsed * 1000)
93+
if sleep:
94+
time.sleep(sleep)
95+
print(
96+
f'{n:>6} | {min(samples):>8.1f} | {statistics.median(samples):>8.1f} | '
97+
f'{max(samples):>8.1f} | {last_size:>8} | {last_status}'
98+
)
99+
print(f' url: {url}')
100+
101+
102+
def main():
103+
parser = argparse.ArgumentParser(description=__doc__)
104+
parser.add_argument('--base-url', default='http://127.0.0.1:8000',
105+
help='Base URL of the running askbot site.')
106+
parser.add_argument('--min-tags', type=int, default=2,
107+
help='Skip threads that carry fewer tags than this.')
108+
parser.add_argument('--max-tags', type=int, default=8,
109+
help='Cap the tag list length (0 = no cap).')
110+
parser.add_argument('--repeats', type=int, default=5,
111+
help='Timed requests per tag count.')
112+
parser.add_argument('--warmups', type=int, default=1,
113+
help='Untimed requests per tag count before timing.')
114+
parser.add_argument('--timeout', type=float, default=60.0,
115+
help='Per-request timeout in seconds.')
116+
parser.add_argument('--sleep', type=float, default=1.5,
117+
help='Pause between requests to avoid the rate limiter.')
118+
args = parser.parse_args()
119+
120+
setup_django()
121+
tags = pick_tag_set(args.min_tags, args.max_tags)
122+
print(f'Base URL: {args.base_url}')
123+
print(f'Repeats: {args.repeats}, warmups: {args.warmups}')
124+
print()
125+
try:
126+
run(args.base_url, tags, args.repeats, args.warmups, args.timeout,
127+
args.sleep)
128+
except urllib.error.URLError as exc:
129+
print(f'Request failed - is the dev server running? {exc}', file=sys.stderr)
130+
sys.exit(1)
131+
132+
133+
if __name__ == '__main__':
134+
main()

0 commit comments

Comments
 (0)