Commit 06afe6ab authored by Vasilii Iakliushin's avatar Vasilii Iakliushin

Memoize rendered markdown between filter calls

Contributes to https://gitlab.com/gitlab-org/gitlab/-/issues/340387

**Problem**

HTML::Pipeline executes a chain of ReferenceFilter. Each filter with
reference cache tries to fetch parents for references and converts
Nokogiri structure into HTML document. This process consumes extra
memory and increases response time.

**Solution**

Use shared `result` variable to store rendered HTML. ReferenceCache
will use it when present or populate when missing.
parent 71d02d1f
---
name: reference_cache_memoization
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71310
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/341849
milestone: '14.4'
type: development
group: group::source code
default_enabled: false
......@@ -11,7 +11,7 @@ module Banzai
def initialize(doc, context = nil, result = nil)
super
@reference_cache = ReferenceCache.new(self, context)
@reference_cache = ReferenceCache.new(self, context, result)
end
# REFERENCE_PLACEHOLDER is used for re-escaping HTML text except found
......
......@@ -7,9 +7,10 @@ module Banzai
include Gitlab::Utils::StrongMemoize
include RequestStoreReferenceCache
def initialize(filter, context)
def initialize(filter, context, result)
@filter = filter
@context = context
@result = result || {}
end
def load_reference_cache(nodes)
......@@ -166,7 +167,7 @@ module Banzai
private
attr_accessor :filter, :context
attr_accessor :filter, :context, :result
delegate :project, :group, :parent, :parent_type, to: :filter
......@@ -184,7 +185,11 @@ module Banzai
end
def prepare_doc_for_scan(doc)
html = doc.to_html
html = if Feature.enabled?(:reference_cache_memoization, project, default_enabled: :yaml)
result[:rendered_html] ||= doc.to_html
else
doc.to_html
end
filter.requires_unescaping? ? unescape_html_entities(html) : html
end
......
......@@ -4,7 +4,7 @@ require 'spec_helper'
RSpec.describe Banzai::CrossProjectReference do
let(:including_class) { Class.new.include(described_class).new }
let(:reference_cache) { Banzai::Filter::References::ReferenceCache.new(including_class, {})}
let(:reference_cache) { Banzai::Filter::References::ReferenceCache.new(including_class, {}, {})}
before do
allow(including_class).to receive(:context).and_return({})
......
......@@ -12,15 +12,48 @@ RSpec.describe Banzai::Filter::References::ReferenceCache do
let(:filter_class) { Banzai::Filter::References::IssueReferenceFilter }
let(:filter) { filter_class.new(doc, project: project) }
let(:cache) { described_class.new(filter, { project: project }) }
let(:cache) { described_class.new(filter, { project: project }, result) }
let(:result) { {} }
describe '#load_references_per_parent' do
subject { cache.load_references_per_parent(filter.nodes) }
it 'loads references grouped per parent paths' do
cache.load_references_per_parent(filter.nodes)
expect(doc).to receive(:to_html).and_call_original
subject
expect(cache.references_per_parent).to eq({ project.full_path => [issue1.iid, issue2.iid].to_set,
project2.full_path => [issue3.iid].to_set })
end
context 'when rendered_html is memoized' do
let(:result) { { rendered_html: 'html' } }
it 'reuses memoized rendered HTML when available' do
expect(doc).not_to receive(:to_html)
subject
end
context 'when feature flag is disabled' do
before do
stub_feature_flags(reference_cache_memoization: false)
end
it 'ignores memoized rendered HTML' do
expect(doc).to receive(:to_html).and_call_original
subject
end
end
end
context 'when result is not available' do
let(:result) { nil }
it { expect { subject }.not_to raise_error }
end
end
describe '#load_parent_per_reference' do
......@@ -47,7 +80,7 @@ RSpec.describe Banzai::Filter::References::ReferenceCache do
it 'does not have an N+1 query problem with cross projects' do
doc_single = Nokogiri::HTML.fragment("#1")
filter_single = filter_class.new(doc_single, project: project)
cache_single = described_class.new(filter_single, { project: project })
cache_single = described_class.new(filter_single, { project: project }, {})
control_count = ActiveRecord::QueryRecorder.new do
cache_single.load_references_per_parent(filter_single.nodes)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment