From 755ca0fcbbf92926f4fc774123c5fb11d0a43945 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 25 Nov 2022 12:40:31 +1100 Subject: [PATCH] PERF: stop downloading images from post processor and lean on uploads Previously we would unconditionally fetch all images via HTTP to grab original sizing from cooked post processor in 2 different spots. This was wasteful as we already calculate and cache this info in upload records. This also simplifies some specs and reduces use of mocks. --- lib/cooked_post_processor.rb | 19 +++++++++++++------ lib/cooked_processor_mixin.rb | 11 +++++++---- spec/fabricators/upload_fabricator.rb | 10 ++++++++++ spec/fixtures/images/2000x2000.png | Bin 0 -> 2451 bytes spec/jobs/pull_hotlinked_images_spec.rb | 9 +++------ spec/lib/cooked_post_processor_spec.rb | 17 ++++++----------- 6 files changed, 39 insertions(+), 27 deletions(-) create mode 100644 spec/fixtures/images/2000x2000.png diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index f3b6e712d76..4d8df118e6a 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -153,14 +153,21 @@ class CookedPostProcessor src = img["src"] return if src.blank? || is_a_hyperlink?(img) || is_svg?(img) - original_width, original_height = (get_size(src) || [0, 0]).map(&:to_i) - if original_width == 0 || original_height == 0 - Rails.logger.info "Can't reach '#{src}' to get its dimension." - return - end - upload = Upload.get_from_url(src) + original_width, original_height = nil + + if (upload.present?) + original_width = upload.width || 0 + original_height = upload.height || 0 + else + original_width, original_height = (get_size(src) || [0, 0]).map(&:to_i) + if original_width == 0 || original_height == 0 + Rails.logger.info "Can't reach '#{src}' to get its dimension." + return + end + end + if (upload.present? && upload.animated?) || src.match?(GIF_SOURCES_REGEXP) img.add_class("animated") end diff --git a/lib/cooked_processor_mixin.rb b/lib/cooked_processor_mixin.rb index e22777ba73b..cf4885788d5 100644 --- a/lib/cooked_processor_mixin.rb +++ b/lib/cooked_processor_mixin.rb @@ -45,10 +45,7 @@ module CookedProcessorMixin oneboxed_images.each do |img| next if img["src"].blank? - src = img["src"].sub(/^https?:/i, "") parent = img.parent - img_classes = (img["class"] || "").split(" ") - link_classes = ((parent&.name == "a" && parent["class"]) || "").split(" ") if respond_to?(:process_hotlinked_image, true) still_an_image = process_hotlinked_image(img) @@ -183,7 +180,13 @@ module CookedProcessorMixin return unless is_valid_image_url?(absolute_url) - @size_cache[url] = FastImage.size(absolute_url) + upload = Upload.get_from_url(absolute_url) + if upload && upload.width && upload.width > 0 + @size_cache[url] = [upload.width, upload.height] + else + @size_cache[url] = FastImage.size(absolute_url) + end + rescue Zlib::BufError, URI::Error, OpenSSL::SSL::SSLError # FastImage.size raises BufError for some gifs, leave it. end diff --git a/spec/fabricators/upload_fabricator.rb b/spec/fabricators/upload_fabricator.rb index 5ccd0b96118..cb47f293b09 100644 --- a/spec/fabricators/upload_fabricator.rb +++ b/spec/fabricators/upload_fabricator.rb @@ -21,6 +21,16 @@ Fabricator(:upload) do extension "png" end +Fabricator(:large_image_upload, from: :upload) do + width 2000 + height 2000 + after_create do |upload, _transients| + file = file_from_fixtures("2000x2000.png") + upload.url = Discourse.store.store_upload(file, upload) + upload.sha1 = Upload.generate_digest(file) + end +end + Fabricator(:image_upload, from: :upload) do transient color: "white" diff --git a/spec/fixtures/images/2000x2000.png b/spec/fixtures/images/2000x2000.png new file mode 100644 index 0000000000000000000000000000000000000000..b0085b18c70343bcec626e4ed63d04ae8ff3362c GIT binary patch literal 2451 zcmeAS@N?(olHy`uVBq!ia0y~yV7~yuj6eZ~P0l~B04bJqM_)$E)e-c@Ne1&9> zAYTTCDm4a%h86~fUqGRT7Yq!g1`G_Z5*Qe)W-u^_7tGleXakgBO7eDhVPL%5CA~;2OC7#SED=^7g98kvL`8Csc`Ss9pV8yHv_7!<93{2N6> xZhlH;S|x4`%-^Ejff_X6Hk4%MrWThZ<`&@A6Y{tU*kWQ}@O1TaS?83{1ONmqKR*Bf literal 0 HcmV?d00001 diff --git a/spec/jobs/pull_hotlinked_images_spec.rb b/spec/jobs/pull_hotlinked_images_spec.rb index 8e1709ef86e..b16915ae428 100644 --- a/spec/jobs/pull_hotlinked_images_spec.rb +++ b/spec/jobs/pull_hotlinked_images_spec.rb @@ -271,7 +271,6 @@ RSpec.describe Jobs::PullHotlinkedImages do post = Fabricate(:post, raw: "") upload.update(access_control_post: Fabricate(:post)) FileStore::S3Store.any_instance.stubs(:store_upload).returns(upload.url) - FastImage.expects(:size).returns([100, 100]).at_least_once expect { Jobs::PullHotlinkedImages.new.execute(post_id: post.id) } .to change { Upload.count }.by(1) @@ -374,7 +373,7 @@ RSpec.describe Jobs::PullHotlinkedImages do post.rebake! post.reload - expect(post.cooked).to match(//) end @@ -542,12 +541,10 @@ RSpec.describe Jobs::PullHotlinkedImages do end describe "with a lightboxed image" do - fab!(:upload) { Fabricate(:upload) } + fab!(:upload) { Fabricate(:large_image_upload) } fab!(:user) { Fabricate(:user) } before do - FastImage.expects(:size).returns([1750, 2000]).at_least_once - OptimizedImage.stubs(:resize).returns(true) Jobs.run_immediately! end diff --git a/spec/lib/cooked_post_processor_spec.rb b/spec/lib/cooked_post_processor_spec.rb index 0d8c7402749..26f1eaededc 100644 --- a/spec/lib/cooked_post_processor_spec.rb +++ b/spec/lib/cooked_post_processor_spec.rb @@ -5,6 +5,7 @@ require "file_store/s3_store" RSpec.describe CookedPostProcessor do fab!(:upload) { Fabricate(:upload) } + fab!(:large_image_upload) { Fabricate(:large_image_upload) } let(:upload_path) { Discourse.store.upload_path } describe "#post_process" do @@ -764,7 +765,6 @@ RSpec.describe CookedPostProcessor do end it "won't remove the original image if another post doesn't have an image" do - FastImage.stubs(:size) topic = post.topic cpp.post_process @@ -781,8 +781,7 @@ RSpec.describe CookedPostProcessor do end it "generates thumbnails correctly" do - FastImage.expects(:size).returns([1750, 2000]) - + # image size in cooked is 1500*2000 topic = post.topic cpp.post_process topic.reload @@ -941,17 +940,13 @@ RSpec.describe CookedPostProcessor do describe "#convert_to_link" do fab!(:thumbnail) { Fabricate(:optimized_image, upload: upload, width: 512, height: 384) } - before do - CookedPostProcessor.any_instance.stubs(:get_size).with(upload.url).returns([1024, 768]) - end - it "adds lightbox and optimizes images" do - post = Fabricate(:post, raw: "![image|1024x768, 50%](#{upload.short_url})") - + post = Fabricate(:post, raw: "![image|1024x768, 50%](#{large_image_upload.short_url})") cpp = CookedPostProcessor.new(post, disable_dominant_color: true) cpp.post_process doc = Nokogiri::HTML5::fragment(cpp.html) + expect(doc.css('.lightbox-wrapper').size).to eq(1) expect(doc.css('img').first['srcset']).to_not eq(nil) end @@ -1001,7 +996,7 @@ RSpec.describe CookedPostProcessor do it "optimizes images in quotes" do post = Fabricate(:post, raw: <<~MD) [quote] - ![image|1024x768, 50%](#{upload.short_url}) + ![image|1024x768, 50%](#{large_image_upload.short_url}) [/quote] MD @@ -1016,7 +1011,7 @@ RSpec.describe CookedPostProcessor do it "optimizes images in Onebox" do Oneboxer.expects(:onebox) .with("https://discourse.org", anything) - .returns("") + .returns("") post = Fabricate(:post, raw: "https://discourse.org")