Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dedupe ancestor title requests and lookup once per field #3025

Merged

Conversation

mark-cooper
Copy link
Member

@mark-cooper mark-cooper commented Jul 27, 2023

This PR addresses 2 issues we encountered with "search" adjacent performance rendering the context cell (particularly with larger page sizes).

  1. Currently a call to get_ancestor_title is made at least twice per "ancestor". Once to see if there's a value, then again to get the value. The PR updates this to just happen once and use the result.

  2. Requests for ancestors are repeated per result (record). If a result shares ancestors the same request is being made without reference to any previous lookups for the same "ancestor". To help with this this PR goes with a minimally intrusive approach of introducing a local variable per controller action. It's simple, can be easily replaced or updated in the future for something more sophisticated (it may eventually be better to do something that isn't requesting an entire json record to get a single field), but does the job of reducing the no. of requests to once per "field" (uri), per search request.

Using the ATTracer repository and searching for "bulk":

master (118 requests):

/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/341
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/341
/repositories/3/archival_objects/342
/repositories/3/archival_objects/342
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/341
/repositories/3/archival_objects/342
/repositories/3/archival_objects/342
/repositories/3/archival_objects/343
/repositories/3/archival_objects/343
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/341
/repositories/3/archival_objects/342
/repositories/3/archival_objects/342
/repositories/3/archival_objects/343
/repositories/3/archival_objects/343
/repositories/3/archival_objects/344
/repositories/3/archival_objects/344
/repositories/3/resources/12
/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/341
/repositories/3/archival_objects/342
/repositories/3/archival_objects/342
/repositories/3/archival_objects/343
/repositories/3/archival_objects/343
/repositories/3/archival_objects/344
/repositories/3/archival_objects/344
/repositories/3/archival_objects/345
/repositories/3/archival_objects/345

PR (11 requests):

/repositories/3/resources/12
/repositories/3/archival_objects/336
/repositories/3/archival_objects/337
/repositories/3/archival_objects/338
/repositories/3/archival_objects/339
/repositories/3/archival_objects/340
/repositories/3/archival_objects/341
/repositories/3/archival_objects/342
/repositories/3/archival_objects/343
/repositories/3/archival_objects/344
/repositories/3/archival_objects/345

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have read the CONTRIBUTING document.
  • I have authority to submit this code.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@brianzelip brianzelip marked this pull request as ready for review September 28, 2023 14:34
@brianzelip
Copy link
Collaborator

@mark-cooper I re-ran CI here a few times, all failed on the same test. I also get the same failure locally.

@mark-cooper
Copy link
Member Author

@brianzelip I'll sync this up with master and see where things stand -- thx for trying.

@mark-cooper mark-cooper force-pushed the dedupe-ancestor-title-requests branch from fab8492 to 3be8dca Compare October 2, 2023 23:35
Copy link
Collaborator

@brianzelip brianzelip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@brianzelip brianzelip merged commit fef1ea7 into archivesspace:master Oct 3, 2023
8 checks passed
@cdibella cdibella added this to the 3.5.0 milestone Oct 4, 2023
mark-cooper added a commit to mark-cooper/archivesspace that referenced this pull request Oct 23, 2023
This updates:
archivesspace#3025

Additional testing has been done to ensure the instance variable
is valid in all views (hence initted in application controller).
thimios pushed a commit that referenced this pull request Apr 4, 2024
This updates:
#3025

Additional testing has been done to ensure the instance variable
is valid in all views (hence initted in application controller).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants