[TNL-5632] Optimize the single_thread view.
Firstly, we're now explicitly instructing the comments service to not return thread responses/comments if the request isn't AJAX. So, if you load the URL for a single discussion thread in your browser, this would be a non-AJAX call and we'll avoid loading the responses for the entire thread behind-the-scenes. Big win here for large threads. Next, we removed a redundant "get threads" call which was also happening behind-the-scenes. This call was redundant as the front-end JS also grabs the thread list when a topic is chosen, so we were making an extranenous call for no benefit. Poof, gone! Finally, we added some caching of database queries that are required to drive a lot of the permissions/cohorts machinery around discussion. This will have a minimal effect but introduced a cleaner way to apply general memoization at the per-request level which will let us further cache things as we identify them as issues.
This commit is contained in:
Toby Lawrence
@@ -4,8 +4,10 @@ Tests for the request cache.
|
||||
from celery.task import task
|
||||
from django.conf import settings
|
||||
from django.test import TestCase
|
||||
from mock import Mock
|
||||
|
||||
from request_cache import get_request_or_stub
|
||||
from request_cache.middleware import RequestCache, request_cached
|
||||
from xmodule.modulestore.django import modulestore
|
||||
|
||||
|
||||
@@ -33,3 +35,190 @@ class TestRequestCache(TestCase):
|
||||
""" Test that the request cache is cleared after a task is run. """
|
||||
self._dummy_task.apply(args=(self,)).get()
|
||||
self.assertEqual(modulestore().request_cache.data, {})
|
||||
|
||||
def test_request_cached_miss_and_then_hit(self):
|
||||
"""
|
||||
Ensure that after a cache miss, we fill the cache and can hit it.
|
||||
"""
|
||||
RequestCache.clear_request_cache()
|
||||
|
||||
to_be_wrapped = Mock()
|
||||
to_be_wrapped.return_value = 42
|
||||
self.assertEqual(to_be_wrapped.call_count, 0)
|
||||
|
||||
def mock_wrapper(*args, **kwargs):
|
||||
"""Simple wrapper to let us decorate our mock."""
|
||||
return to_be_wrapped(*args, **kwargs)
|
||||
|
||||
wrapped = request_cached(mock_wrapper)
|
||||
result = wrapped()
|
||||
self.assertEqual(result, 42)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
result = wrapped()
|
||||
self.assertEqual(result, 42)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
def test_request_cached_with_caches_despite_changing_wrapped_result(self):
|
||||
"""
|
||||
Ensure that after caching a result, we always send it back, even if the underlying result changes.
|
||||
"""
|
||||
RequestCache.clear_request_cache()
|
||||
|
||||
to_be_wrapped = Mock()
|
||||
to_be_wrapped.side_effect = [1, 2, 3]
|
||||
self.assertEqual(to_be_wrapped.call_count, 0)
|
||||
|
||||
def mock_wrapper(*args, **kwargs):
|
||||
"""Simple wrapper to let us decorate our mock."""
|
||||
return to_be_wrapped(*args, **kwargs)
|
||||
|
||||
wrapped = request_cached(mock_wrapper)
|
||||
result = wrapped()
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
result = wrapped()
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
direct_result = mock_wrapper()
|
||||
self.assertEqual(direct_result, 2)
|
||||
self.assertEqual(to_be_wrapped.call_count, 2)
|
||||
|
||||
result = wrapped()
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 2)
|
||||
|
||||
direct_result = mock_wrapper()
|
||||
self.assertEqual(direct_result, 3)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
def test_request_cached_with_changing_args(self):
|
||||
"""
|
||||
Ensure that calling a decorated function with different positional arguments
|
||||
will not use a cached value invoked by a previous call with different arguments.
|
||||
"""
|
||||
RequestCache.clear_request_cache()
|
||||
|
||||
to_be_wrapped = Mock()
|
||||
to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6]
|
||||
self.assertEqual(to_be_wrapped.call_count, 0)
|
||||
|
||||
def mock_wrapper(*args, **kwargs):
|
||||
"""Simple wrapper to let us decorate our mock."""
|
||||
return to_be_wrapped(*args, **kwargs)
|
||||
|
||||
wrapped = request_cached(mock_wrapper)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(1)
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(2)
|
||||
self.assertEqual(result, 2)
|
||||
self.assertEqual(to_be_wrapped.call_count, 2)
|
||||
|
||||
# This is bypass of the decorator.
|
||||
direct_result = mock_wrapper(3)
|
||||
self.assertEqual(direct_result, 3)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
# These will be hits, and not make an underlying call.
|
||||
result = wrapped(1)
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
result = wrapped(2)
|
||||
self.assertEqual(result, 2)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
def test_request_cached_with_changing_kwargs(self):
|
||||
"""
|
||||
Ensure that calling a decorated function with different keyword arguments
|
||||
will not use a cached value invoked by a previous call with different arguments.
|
||||
"""
|
||||
RequestCache.clear_request_cache()
|
||||
|
||||
to_be_wrapped = Mock()
|
||||
to_be_wrapped.side_effect = [1, 2, 3, 4, 5, 6]
|
||||
self.assertEqual(to_be_wrapped.call_count, 0)
|
||||
|
||||
def mock_wrapper(*args, **kwargs):
|
||||
"""Simple wrapper to let us decorate our mock."""
|
||||
return to_be_wrapped(*args, **kwargs)
|
||||
|
||||
wrapped = request_cached(mock_wrapper)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(1, foo=1)
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(2, foo=2)
|
||||
self.assertEqual(result, 2)
|
||||
self.assertEqual(to_be_wrapped.call_count, 2)
|
||||
|
||||
# This is bypass of the decorator.
|
||||
direct_result = mock_wrapper(3, foo=3)
|
||||
self.assertEqual(direct_result, 3)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
# These will be hits, and not make an underlying call.
|
||||
result = wrapped(1, foo=1)
|
||||
self.assertEqual(result, 1)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
result = wrapped(2, foo=2)
|
||||
self.assertEqual(result, 2)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
# Since we're changing foo, this will be a miss.
|
||||
result = wrapped(2, foo=5)
|
||||
self.assertEqual(result, 4)
|
||||
self.assertEqual(to_be_wrapped.call_count, 4)
|
||||
|
||||
def test_request_cached_with_none_result(self):
|
||||
"""
|
||||
Ensure that calling a decorated function that returns None
|
||||
properly caches the result and doesn't recall the underlying
|
||||
function.
|
||||
"""
|
||||
RequestCache.clear_request_cache()
|
||||
|
||||
to_be_wrapped = Mock()
|
||||
to_be_wrapped.side_effect = [None, None, None, 1, 1]
|
||||
self.assertEqual(to_be_wrapped.call_count, 0)
|
||||
|
||||
def mock_wrapper(*args, **kwargs):
|
||||
"""Simple wrapper to let us decorate our mock."""
|
||||
return to_be_wrapped(*args, **kwargs)
|
||||
|
||||
wrapped = request_cached(mock_wrapper)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(1)
|
||||
self.assertEqual(result, None)
|
||||
self.assertEqual(to_be_wrapped.call_count, 1)
|
||||
|
||||
# This will be a miss, and make an underlying call.
|
||||
result = wrapped(2)
|
||||
self.assertEqual(result, None)
|
||||
self.assertEqual(to_be_wrapped.call_count, 2)
|
||||
|
||||
# This is bypass of the decorator.
|
||||
direct_result = mock_wrapper(3)
|
||||
self.assertEqual(direct_result, None)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
# These will be hits, and not make an underlying call.
|
||||
result = wrapped(1)
|
||||
self.assertEqual(result, None)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
result = wrapped(2)
|
||||
self.assertEqual(result, None)
|
||||
self.assertEqual(to_be_wrapped.call_count, 3)
|
||||
|
||||
Reference in New Issue
Block a user