[#3484] Add missing argument to Hash default proc #3485
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The proc to fill in a default value for a Hash accepts two arguments: the hash itself and then the key. Prior to this commit, our proc was only using the Hash, but was treating it as if it was a key. This had the potential to cause some serious performance problems when solr returned many facets or many facet values, when a huge hash was treated as a string and interpolated into other strings via Hash#inspect. Adding this missing argument ensures that we send the key/field_name, rather than the entire hash. For the provided test case on my laptop, this commit speed up null facet object generation up from 45 iterations/second to 18,700 iterations/second. It also doubles the performance of our advanced search form. This commit also adds the rspec-benchmark gem as a dev dependency -- I'm definitely open to figuring out a different way to test this if the additional dependency is not desired. Closes #3484 Co-authored-by: Christina Chortaria <[email protected]> Co-authored-by: Kevin Reiss <[email protected]> Co-authored-by: Max Kadel <[email protected]>
sandbergja
added a commit
to pulibrary/orangelight
that referenced
this pull request
Jan 4, 2025
…ch: true Before this commit, the AdvancedSearchFormComponent processed any field in the CatalogController that did not have include_in_advanced_search: false. In theory, this should not be particularly expensive, but a subtle bug in Blacklight causes some performance issues in cases where we try to process fields that aren't included in the Solr response and when the solr response contains many facet values -- basically the exact situation of our advanced search form! See projectblacklight/blacklight#3484 and projectblacklight/blacklight#3485 for further details.
maxkadel
approved these changes
Jan 6, 2025
There was a problem hiding this comment.
Thanks @sandbergja!
I have one comment, but it does not need to block this from going in.
Having seen loading single tests get slow in other applications, I'm starting to think more conservatively about loading dependencies for the whole suite.
It's really cool to learn about rspec-benchmark! I'm also excited about the advanced search form loading faster!
…pec that actually needs it
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The proc to fill in a default value for a Hash accepts two arguments: the hash itself and then the key. Prior to this commit, our proc was only using the Hash, but was treating it as if it was a key. This had the potential to cause some serious performance problems when solr returned many facets or many facet values, when a huge hash was treated as a string and interpolated into other strings via
Hash#inspect.Adding this missing argument ensures that we send the key/field_name, rather than the entire hash. For the provided test case on my laptop, this commit speed up null facet object generation up from 45 iterations/second to 18,700 iterations/second. It also doubles the performance of our advanced search form.
This commit also adds the rspec-benchmark gem as a dev dependency -- I'm definitely open to figuring out a different way to test this if the additional dependency is not desired.
Closes #3484