Apache Allura™ / Tickets / #4359 Reduce duplicate queries in threaded discussion display

#4359 Reduce duplicate queries in threaded discussion display

Milestone: v1.14.0

Status: closed

Owner: Kenton Taylor

Labels: performance (126)

Component: General

Reviewer: Dave Brondsema

Updated: 2022-09-23

Created: 2012-06-12

Creator: Dave Brondsema

Private: 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={}...

Dave Brondsema - 2012-10-01

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

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

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,
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Kenton Taylor - 2022-03-29

status: open --> review
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2022-03-30

status: review --> closed

assigned_to: Kenton Taylor

Reviewer: Dave Brondsema
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2022-09-23

Milestone: unreleased --> v1.14.0
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Apache Allura™

Forge software for hosting software projects

Milestone

Searches

Help

#4359 Reduce duplicate queries in threaded discussion display

Related

Discussion