Cache MiqExpression.get_col_type in MiqReport::Formatting #17195
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
NickLaMuro
force-pushed
the
cache_miq_report_format_column_type_lookup
branch
from
f20a3aa to
3862d36
Compare
This call will make the same call to the DB over and over again on certain attributes, and is incredibly "N+1y" on large reports. This adds a simple cache for the data return from the DB calls to make sure they are only executed once per column. Moved into a separate method to break up the bulk of the current method. Note: An extra `to_sym` was removed when in the new method since it would always get executed on the next line if the value existed. Probably could remove the `unless` as well since at that point, as we should be clear of any `nil` errors... 🤷
|
@miq-bot assign @gtanzillo Gregg, figured I would assign this to you since the git history has your fingerprints all over it ;) Feel free to re-assign if you have someone better in mind to look at this. Thanks. |
kbrock
approved these changes
Mar 28, 2018
|
Checked commit NickLaMuro@3862d36 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
gtanzillo
approved these changes
Apr 2, 2018
|
@gtanzillo Thanks! Based on the BZ that this is for, I think we want to backport this to the G release. That seem reasonable? |
👍 I added the necessary labels |
|
Thanks! |
1 task
simaishi
pushed a commit
that referenced
this pull request
Apr 10, 2018
…umn_type_lookup Cache MiqExpression.get_col_type in MiqReport::Formatting (cherry picked from commit 1b18fb1) https://bugzilla.redhat.com/show_bug.cgi?id=1565677
|
Gaprindashvili backport details: |
simaishi
pushed a commit
that referenced
this pull request
Apr 10, 2018
…umn_type_lookup Cache MiqExpression.get_col_type in MiqReport::Formatting (cherry picked from commit 1b18fb1) https://bugzilla.redhat.com/show_bug.cgi?id=1565678
|
Fine backport details: |
d-m-u
pushed a commit
to d-m-u/manageiq
that referenced
this pull request
Jun 6, 2018
…rmat_column_type_lookup Cache MiqExpression.get_col_type in MiqReport::Formatting (cherry picked from commit 1b18fb1) https://bugzilla.redhat.com/show_bug.cgi?id=1565678
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.
I noticed while running a report that there were a lot of
0 rowqueries being executed against the DB when running a report. Turns out that when a report is building thehtmldata, theMiqReport::Formattingcode is checking for every column in every row what the format should be, even though it shouldn't change on a row by row basis.Specifically, this becomes a problem when it uses
MiqExpression.get_col_typeto determine the format as it will fetch from the DB what the column type is to help determine the format. This query does not get cached, so it is duplicated for every column in each row of the report dataset.The query in my tests (from the report described in #17141 ) that was being repeated in the logs was as follows:
Benchmarks
Using the same dataset and report from #17141 , the difference before and after this change is as follows:
Of note: The changes from #17141 were in place in both runs of the above.
Links