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.

Sign up for GitHub

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

Jump to bottom

Accessing Null Facet Fields through @response.aggregations can be very slow #3484

Closed
sandbergja opened this issue Jan 3, 2025 · 0 comments · Fixed by #3485
Closed

Accessing Null Facet Fields through @response.aggregations can be very slow #3484

sandbergja opened this issue Jan 3, 2025 · 0 comments · Fixed by #3485

Comments

@sandbergja
Copy link
Contributor

When you try to access a facet field that is not in the solr response, e.g. @response.aggregations['my-new-field'], Blacklight returns a null Blacklight::Solr::Response::FacetField object.

@maxkadel , @kevinreiss, @christinach , and I were investigating a performance issue in our advanced search form, and found that every time we try to do @response.aggregations['my-new-field'] on a field not in the solr response, the code spends a lot of time inHash#inspect mysteriously. It turns out that there is a subtle bug in the default_aggregations method that causes another method called facet_field_aggregation_options to receive a huge hash of all facets and their values, rather than the field name string that it expects. It tries to interpolate the hash in a few places the field_name should go via Hash#inspect, and it can get pretty expensive if you have a lot of facet values to deal with.

PR coming up soon!
sandbergja added a commit that referenced this issue Jan 3, 2025
@sandbergja
[#3484] Add missing argument to Hash default proc
5808b67 
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
sandbergja added a commit that referenced this issue Jan 3, 2025
@sandbergja @christinach
@kevinreiss @maxkadel
[#3484] Add missing argument to Hash default proc
e03b66a 
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 sandbergja mentioned this issue Jan 3, 2025
Merged
sandbergja added a commit to pulibrary/orangelight that referenced this issue Jan 4, 2025
@sandbergja
Advanced form: only process facets that have include_in_advanced_sear…
a186e39 
…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.
@sandbergja sandbergja mentioned this issue Jan 4, 2025
Merged
maxkadel added a commit that referenced this issue Jan 6, 2025
@maxkadel
Merge pull request #3485 from projectblacklight/improve-performance-o…
bdc3c86 
…f-null-objects

[#3484] Add missing argument to Hash default proc
sandbergja added a commit that referenced this issue Jan 6, 2025
@sandbergja @christinach
@kevinreiss @maxkadel
[#3484] Add missing argument to Hash default proc
9ecefa3 
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 that referenced this issue Jan 7, 2025
@sandbergja @christinach
@kevinreiss @maxkadel
[#3484] Add missing argument to Hash default proc (#3486)
03ea265 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#3484] Add missing argument to Hash default proc projectblacklight/blacklight
1 participant
@sandbergja