#4359 Reduce duplicate queries in threaded discussion display

v1.14.0
closed
General
2022-09-23
2012-06-12
No

There are lot of duplicate ming queries for a threaded discussion. The following is logged by timermiddleware, when viewing a ticket page that had 2 comments by admin1, when logged in as admin1.

get args=(<class 'allura.model.auth.User'>, ObjectId('4fbc01a69c104009d2000407')) kwargs={}
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.ProjectFile'>, {'category': 'icon', 'project_id': ObjectId('4fbc01a69c104009d2000408')}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.ProjectFile'>, {'category': 'icon', 'project_id': ObjectId('4fbc01a69c104009d2000408')}) kwargs={}...
get args=(<class 'allura.model.auth.User'>, ObjectId('4fbc01a69c104009d2000407')) kwargs={}
get args=(<class 'allura.model.auth.User'>, ObjectId('4fbc01a69c104009d2000407')) kwargs={}
get args=(<class 'allura.model.index.ArtifactReference'>, u'allura/model/discuss/Post#6f62e5c2eec5ff46a51fb65a4d91efcba41b1aaf/tickets@testall/p/sourceforge/net') kwargs={}...
find args=(<class 'allura.model.index.ArtifactReference'>, {'references': u'allura/model/discuss/Post#6f62e5c2eec5ff46a51fb65a4d91efcba41b1aaf/tickets@testall/p/sourceforge/net'}) kwargs={}...
get args=(<class 'allura.model.discuss.Thread'>, u'f0cd42a4') kwargs={}
get args=(<class 'allura.model.discuss.Discussion'>, ObjectId('4fbc059b9c104012b2000030')) kwargs={}
get args=(<class 'allura.model.project.AppConfig'>, ObjectId('4fbc059b9c104012b200002e')) kwargs={}
get args=(<class 'allura.model.project.AppConfig'>, ObjectId('4fbc059b9c104012b200002e')) kwargs={}
find args=(<class 'allura.model.discuss.DiscussionAttachment'>, {'post_id': u'6f62e5c2eec5ff46a51fb65a4d91efcba41b1aaf.tickets@testall.p.sourceforge.net', 'type': 'attachment'}) kwargs={}...
get args=(<class 'allura.model.auth.User'>, None) kwargs={}
get args=(<class 'allura.model.index.ArtifactReference'>, u'forgetracker/model/ticket/Ticket#4fbd3f079c1040225a000002') kwargs={}...
get args=(<class 'allura.model.project.Project'>, ObjectId('4fbc059b9c104012b2000000')) kwargs={}
find args=(<class 'allura.model.project.AppConfig'>, {'options.mount_point': None, 'project_id': ObjectId('4fbc059b9c104012b2000000')}) kwargs={}...
get args=(<class 'forgetracker.model.ticket.Ticket'>, ObjectId('4fbd3f079c1040225a000002')) kwargs={}
get args=(<class 'allura.model.discuss.Post'>, None) kwargs={}
get args=(<class 'allura.model.auth.User'>, ObjectId('4fbc01a69c104009d2000407')) kwargs={}
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.ProjectFile'>, {'category': 'icon', 'project_id': ObjectId('4fbc01a69c104009d2000408')}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...
find args=(<class 'allura.model.project.Project'>, {'deleted': False, 'shortname': u'u/admin1'}) kwargs={}...

Related

Tickets: #8422

Discussion

  • Dave Brondsema

    Dave Brondsema - 2012-10-01

    The user-icon portions of these queries have been addressed; those are cached as of [9667df]

     
  • Kenton Taylor - 2022-03-23
    • Allura/allura/model/discuss.py:find_posts:
      short circuit if self.posts is already present and avoid requerying

    • artifactreference bulk preload and cache
      this was the biggest; each post has to check for backreferenced artifacts before checking for related artifacts. bulk querying for all these and caching them as instance attrs allows
      us to skip all the individual checks

    • ForgeDiscussion/forgediscussion/model/forum.py:ForumThread.subscribed
      it's super() call was memoized, but this invocation wasn't

    • memoize Allura/allura/model/notification.py:Mailbox.subscribed
      i believe this shaved one ming call off; not much but can't hurt

    • edit post HTML isn't present when not logged in, so nothing to be done there

    • there still seem to be more Project fetches than should be needed, but didn't really find a good way to identify truly unnecessary ones or reduce those

    • final counts - 6 total posts, 2 top level, 1 related artifact and one backrefd
      before: mongo": 172, "ming": 104,
      after: mongo": 135, "ming": 84,

     
  • Dave Brondsema

    Dave Brondsema - 2022-03-23
    • find_posts - self.posts is a RelationProperty('Post', via='thread_id') which seems like it'd always exist and contain all associated posts. And not be using the page, limit, timestamp, style params? Test failure maybe related:
    ======================================================================
    FAIL: allura.tests.model.test_discussion.test_thread_methods
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
        self.test(*self.arg)
      File "/src/allura/AlluraTest/alluratest/tools.py", line 32, in with_setup__decorated
        return func(*a, **kw)
      File "/src/allura/Allura/allura/tests/model/test_discussion.py", line 96, in test_thread_methods
        assert posts0 != posts1
    AssertionError
    
    • artifactreference bulk load - can we do it for all threads, not just the discussion tool? tickets, merge requests, etc
      • allura.lib.widgets.discuss:Thread i think is the widget used on all of them
      • def prepare_context is a main lifecycle method on widgets, can the bulk load happen there?
    • def subscribed using @memoize
      • seems kinda bad since it has a self param and user_id param its going to be a really big forever cache. Maybe ok though since we're already doing it in a very similar place.
      • Another concern is that it'll return the wrong value if someone changes their subscription status, and then it is still cached. Maybe its only ok on the Artifact & ForumThread since new copies of those objects get loaded up frequently, but not ok on the Mailbox since its on a @classmethod?
      • related test failures:
    ======================================================================
    FAIL: forgewiki.tests.functional.test_root.TestRootController.test_subscribe
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
        self.test(*self.arg)
      File "/src/allura/ForgeWiki/forgewiki/tests/functional/test_root.py", line 846, in test_subscribe
        assert M.Mailbox.subscribed(user_id=user._id)
    AssertionError
    ======================================================================
    FAIL: Newly added admin must be subscribed to all the tools in the project
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
        self.test(*self.arg)
      File "/src/allura/Allura/allura/tests/functional/test_admin.py", line 679, in test_new_admin_subscriptions
        assert not sub, 'New admin not unsubscribed to app %s' % ac
    AssertionError: New admin not unsubscribed to app <AppConfig _id=ObjectId('623b38800552214ce15e52e9')
      project_id=ObjectId('623b38800552214ce15e52e1')
      discussion_id=None tool_name='admin' version=None
      options=I{'mount_point': 'admin', 'mount_label': 'Admin',
      'ordinal': 0} tool_data=I{} acl=I[]>
    
    ======================================================================
    FAIL: allura.tests.functional.test_auth.TestAuth.test_prefs_subscriptions_subscribe
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
        self.test(*self.arg)
      File "/src/allura/Allura/allura/tests/decorators.py", line 48, in wrapped
        return func(*args, **kw)
      File "/src/allura/Allura/allura/tests/functional/test_auth.py", line 904, in test_prefs_subscriptions_subscribe
        assert subscribed, "User is not subscribed for tool %s" % t_id
    AssertionError: User is not subscribed for tool 623b38a2db62bbd2f35e63f7
    
    • edit post HTML - ok, yep must've been fixed on some other ticket since this one was reported
    • another test failure, not sure what part its related to:
    FAIL: forgetracker.tests.functional.test_root.TestFunctionalController.test_delete_attachment_from_comments
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/var/local/env-allura/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
        self.test(*self.arg)
      File "/src/allura/ForgeTracker/forgetracker/tests/functional/test_root.py", line 938, in test_delete_attachment_from_comments
        assert '<i class="fa fa-trash-o" aria-hidden="true"></i>' not in r
    AssertionError
    
    • I think the Project fetches are ok (at least not the biggest target). I think the biggest impact is queries that scale up with the number of comments on a thread, since they could be really bad when there are dozens of responses. I made a thread with 6 comments and found the following. Up to you if you want to tackle now, or we can make a separate ticket
      • 7 ArtifactReference queries, maybe these are regular ("forward") references from def related_artifacts calling self.refs (probably 1 extra query for the thread itself)
      • 5 ForumAttachment queries. (no idea why not 6)
     
    • Kenton Taylor - 2022-03-29
      • indeed, removed
      • we certainly could, but I'm thinking that should be its own separate ticket
      • As discussed our memoize decorator doesn't suffer from that memory bloat problem. However, the usage on the @classmethod was an issue, so that's been removed, and tests are passing now.

      New call counts are: "ming": 85, "mongo": 135.

       
  • Kenton Taylor - 2022-03-29
    • status: open --> review
     
  • Dave Brondsema

    Dave Brondsema - 2022-03-30
    • status: review --> closed
    • assigned_to: Kenton Taylor
    • Reviewer: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2022-09-23
    • Milestone: unreleased --> v1.14.0
     

Log in to post a comment.