From eab14e31fdf445b1620d1cf01867c1697cfd3313 Mon Sep 17 00:00:00 2001 From: Matchu Date: Mon, 28 Jan 2013 15:18:11 -0600 Subject: [PATCH] waaay speed up Pet#translate_items A few key changes: * Don't reload the whole pet 8 times!! Sooo many bad things happen, including redundant lookups of everything else and too many item saves and reindexes. Instead, fetch the item data, apply it to the items, and then save the items (once each!) * Updated my branch of globalize3 to be even better at avoiding redundant queries when saving. Woo. * Last realization: wrapping all the item saves in a single transaction works wonders. COMMIT seems to have high overhead, so doing only one took it from 50ms * 10 or whatever to 60ms. Good stuff. --- Gemfile.lock | 2 +- app/models/pet.rb | 117 +++++++++++++++++++++++++++++----------------- 2 files changed, 74 insertions(+), 45 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index ed178087..6c9533a4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -46,7 +46,7 @@ GIT GIT remote: git://github.com/matchu/globalize3.git - revision: c7cfc359f9fc8c283e45b3847ec9b9aaedff1d54 + revision: e23277864270131bbee29033f8a3abfb74ffe1f8 specs: globalize3 (0.3.0) activemodel (>= 3.0.0) diff --git a/app/models/pet.rb b/app/models/pet.rb index afd2f2a6..eb3c26c1 100644 --- a/app/models/pet.rb +++ b/app/models/pet.rb @@ -1,4 +1,5 @@ require 'rocketamf/remote_gateway' +require 'ostruct' class Pet < ActiveRecord::Base GATEWAY_URL = 'http://www.neopets.com/amfphp/gateway.php' @@ -21,47 +22,46 @@ class Pet < ActiveRecord::Base options[:locale] ||= I18n.default_locale I18n.with_locale(options[:locale]) do - require 'ostruct' - begin - neopets_language_code = I18n.compatible_neopets_language_code_for(options[:locale]) - envelope = PET_VIEWER.request([name, 0]).post( - :timeout => 2, - :headers => { - 'Cookie' => "lang=#{neopets_language_code}" - } - ) - rescue RocketAMF::RemoteGateway::AMFError => e - if e.message == PET_NOT_FOUND_REMOTE_ERROR - raise PetNotFound, "Pet #{name.inspect} does not exist" - end - raise DownloadError, e.message - rescue RocketAMF::RemoteGateway::ConnectionError => e - raise DownloadError, e.message - end - contents = OpenStruct.new(envelope.messages[0].data.body) - pet_data = OpenStruct.new(contents.custom_pet) + viewer_data = fetch_viewer_data + pet_data = OpenStruct.new(viewer_data.custom_pet) - # in case this is running in a thread, explicitly grab an ActiveRecord - # connection, to avoid connection conflicts - Pet.connection_pool.with_connection do - self.pet_type = PetType.find_or_initialize_by_species_id_and_color_id( - pet_data.species_id.to_i, - pet_data.color_id.to_i - ) - self.pet_type.body_id = pet_data.body_id - self.pet_type.origin_pet = self - biology = pet_data.biology_by_zone - biology[0] = nil # remove effects if present - @pet_state = self.pet_type.add_pet_state_from_biology! biology - @pet_state.label_by_pet(self, pet_data.owner) - @items = Item.collection_from_pet_type_and_registries(self.pet_type, - contents.object_info_registry, contents.object_asset_registry, - options[:item_scope]) - end + self.pet_type = PetType.find_or_initialize_by_species_id_and_color_id( + pet_data.species_id.to_i, + pet_data.color_id.to_i + ) + self.pet_type.body_id = pet_data.body_id + self.pet_type.origin_pet = self + biology = pet_data.biology_by_zone + biology[0] = nil # remove effects if present + @pet_state = self.pet_type.add_pet_state_from_biology! biology + @pet_state.label_by_pet(self, pet_data.owner) + @items = Item.collection_from_pet_type_and_registries(self.pet_type, + viewer_data.object_info_registry, viewer_data.object_asset_registry, + options[:item_scope]) end true end + + def fetch_viewer_data + begin + neopets_language_code = I18n.compatible_neopets_language_code_for(I18n.locale) + envelope = PET_VIEWER.request([name, 0]).post( + :timeout => 2, + :headers => { + 'Cookie' => "lang=#{neopets_language_code}" + } + ) + rescue RocketAMF::RemoteGateway::AMFError => e + if e.message == PET_NOT_FOUND_REMOTE_ERROR + raise PetNotFound, "Pet #{name.inspect} does not exist" + end + raise DownloadError, e.message + rescue RocketAMF::RemoteGateway::ConnectionError => e + raise DownloadError, e.message + end + OpenStruct.new(envelope.messages[0].data.body) + end def wardrobe_query { @@ -99,16 +99,45 @@ class Pet < ActiveRecord::Base candidates = self.item_translation_candidates until candidates.empty? - last_pet_loaded = nil - reloaded_pets = Parallel.map(candidates.keys, :in_threads => 8) do |locale| - Rails.logger.info "Reloading #{name} in #{locale}" - reloaded_pet = Pet.load(name, :item_scope => Item.includes(:translations), - :locale => locale) - last_pet_loaded = reloaded_pet + # Organize known items by ID + items_by_id = {} + @items.each { |i| items_by_id[i.id] = i } + + # Fetch registry data in parallel + registries = Parallel.map(candidates.keys, :in_threads => 8) do |locale| + viewer_data = I18n.with_locale(locale) { fetch_viewer_data } + [locale, viewer_data.object_info_registry] end - reloaded_pets.each(&:save!) + + # Look up any newly applied items on this pet, just in case + new_item_ids = [] + registries.each do |locale, registry| + registry.each do |item_id, item_info| + item_id = item_id.to_i + new_item_ids << item_id unless items_by_id.has_key?(item_id) + end + end + Item.includes(:translations).find(new_item_ids).each do |item| + items_by_id[item.id] = item + end + + # Apply translations, and figure out what items are currently being worn + current_items = [] + registries.each do |locale, registry| + I18n.with_locale(locale) do + registry.each do |item_id, item_info| + item = items_by_id[item_id.to_i] + item.origin_registry_info = item_info + current_items << item + end + end + end + + @items = current_items + Item.transaction { @items.each { |i| i.save! if i.changed? } } + previous_candidates = candidates - candidates = last_pet_loaded.item_translation_candidates + candidates = item_translation_candidates if previous_candidates == candidates # This condition should never happen if Neopets responds with correct