Optimize number of transactions sent in refresh #14670
Conversation
Allow save inventory to skip starting a transaction if the record being saved was not actually changed by the provider's inventory data. VMs and Storages still directly call `update_attributes!`. At the moment, it doesn't seem like those object counts are limiting factors. If that changes, we can look at applying the same change there.
Do not do a blank transaction if the record hasn't changed, becaiuse it causes extra 2 queries, transaction BEGIN and END
Wrap updating&deleting in 1 big transaction. Doing transaction per update is expensive, since we do BEGIN, update, END, so 3 queries per one updateand the same for delete. With remote DB, this will add a lot of processing time.
|
I own a commit in this PR, so assigning over to @Fryguy |
| deletes.delete(found) unless deletes.blank? | ||
| end | ||
| found | ||
| end | ||
|
|
||
| def update_attributes!(ar_model, attributes, remove_keys) | ||
| ar_model.assign_attributes(attributes.except(*remove_keys)) | ||
| ar_model.save! if ar_model.changed? |
There was a problem hiding this comment.
Can you put a note here with a reference to the upstream issue? Maybe something like
# HACK: Avoid empty BEGIN/COMMIT pair until fix is made for https://github.com/rails/rails/issues/17937
|
@Ladas Can you show some statistic on improvement? Matrix of Remote DB/Local DB x Initial refresh/second refresh |
| type = association.proxy_association.reflection.name | ||
| _log.info("[#{type}] Deleting #{log_format_deletes(deletes)}") | ||
| disconnect ? deletes.each(&:disconnect_inv) : association.delete(deletes) | ||
| end |
There was a problem hiding this comment.
If there are no deletes, doesn't this introduce an empty BEGIN/COMMIT pair because of the transaction? Instead, is it better to do
unless deletes.blank?
ActiveRecord::Base.transaction do
type = association.proxy_association.reflection.name
_log.info("[#{type}] Deleting #{log_format_deletes(deletes)}")
disconnect ? deletes.each(&:disconnect_inv) : association.delete(deletes)
end
endThere was a problem hiding this comment.
Good catch, will fix it. Though this will not affect the stats much, due to the nested nature.
|
Overall looks good...couple of minor things. |
Add a comment about rails issue causing nil transaction
Do a transaction only if there are items to be deleted, to avoind empty transactions.
|
Checked commits Ladas/manageiq@8ad0ac2~...2b2862e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@Fryguy I've fixed the minor things and added the table showing the performance improvement in % |
|
@miq-bot add_labels performance |
…sent_in_refresh Optimize number of transactions sent in refresh (cherry picked from commit 73555f3)
|
Fine backport details: |
|
Backported to Euwe via #14674 |
Do not do a blank transaction if the record hasn't changed, because it causes extra 2 queries, transaction BEGIN and END. And wrap updating&deleting in 1 big transaction. Doing transaction per update is expensive, since we do BEGIN, update, END, so 3 queries per one updateand the same for delete. With remote DB, this will add a lot of processing time.
Example:
Partially fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1436176
The performance improvements are mainly visible with second+ refresh, since creating objects by association.push(new_records) is already using 1 transaction for many records created. We are just optimizing that number of transaction also for nested objects created.
** stats are being measured with applied PRs #13594 #14542
Performance improvements, given we are saving 290k object to the DB during refresh:
** the second refresh is measured when nothing is updated (the same VCR). But this PR brings a big speedup when we do update/delete records, since we do only 1 query per updated/deleted item, not 3 queries, like we did before this PR.