summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDouwe Maan <douwe@gitlab.com>2016-04-04 10:14:56 +0000
committerDouwe Maan <douwe@gitlab.com>2016-04-04 10:14:56 +0000
commit4bf302799f7bd32e27c0fe83abe34239d9c3e1fd (patch)
treedf691dca4b9768ffc5b2ab66ca1571fb59e18510
parent3dd91f55a048c13e6dc81ec685e521d2fc5a78f5 (diff)
parent915fd3f910921bc97abdda5f2ae63093c11533fd (diff)
downloadgitlab-ce-4bf302799f7bd32e27c0fe83abe34239d9c3e1fd.tar.gz
Merge branch 'banzai-webscale' into 'master'
Improve rendering performance of Banzai Related issues: * gitlab-org/gitlab-ce#14315 * gitlab-org/gitlab-ce#13651 TODO: * [x] `Banzai::Filter::ExternalIssueReferenceFilter#call` calls `Project#default_issues_tracker?` which runs a query on every call. Memoizing this only works if the `Project` instance is shared between instances of this filter. Either way it should at most only run this method once. * [x] Fix the two failing specs * [x] Clean of the various `call` methods in the filters where I inlined methods * [x] Remove the `replace_XXX` methods in ReferenceFilter now that they're no longer used. See merge request !3389
-rw-r--r--CHANGELOG1
-rw-r--r--Gemfile2
-rw-r--r--Gemfile.lock4
-rw-r--r--lib/banzai/filter/abstract_reference_filter.rb140
-rw-r--r--lib/banzai/filter/external_issue_reference_filter.rb41
-rw-r--r--lib/banzai/filter/reference_filter.rb148
-rw-r--r--lib/banzai/filter/user_reference_filter.rb25
7 files changed, 215 insertions, 146 deletions
diff --git a/CHANGELOG b/CHANGELOG
index fc5e06ed94d..f8f21ed0dc9 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
v 8.7.0 (unreleased)
- Don't attempt to fetch any tags from a forked repo (Stan Hu)
+ - Improved Markdown rendering performance !3389 (Yorick Peterse)
- Don't attempt to look up an avatar in repo if repo directory does not exist (Stan hu)
- Preserve time notes/comments have been updated at when moving issue
- Make HTTP(s) label consistent on clone bar (Stan Hu)
diff --git a/Gemfile b/Gemfile
index 006e53e0c10..6327227282a 100644
--- a/Gemfile
+++ b/Gemfile
@@ -214,7 +214,7 @@ gem 'jquery-rails', '~> 4.0.0'
gem 'jquery-scrollto-rails', '~> 1.4.3'
gem 'jquery-ui-rails', '~> 5.0.0'
gem 'raphael-rails', '~> 2.1.2'
-gem 'request_store', '~> 1.2.0'
+gem 'request_store', '~> 1.3.0'
gem 'select2-rails', '~> 3.5.9'
gem 'virtus', '~> 1.0.1'
gem 'net-ssh', '~> 3.0.1'
diff --git a/Gemfile.lock b/Gemfile.lock
index bd41cc84198..229089f431d 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -652,7 +652,7 @@ GEM
redis-store (~> 1.1.0)
redis-store (1.1.7)
redis (>= 2.2)
- request_store (1.2.1)
+ request_store (1.3.0)
rerun (0.11.0)
listen (~> 3.0)
responders (2.1.1)
@@ -1011,7 +1011,7 @@ DEPENDENCIES
redcarpet (~> 3.3.3)
redis-namespace
redis-rails (~> 4.0.0)
- request_store (~> 1.2.0)
+ request_store (~> 1.3.0)
rerun (~> 0.11.0)
responders (~> 2.0)
rouge (~> 1.10.1)
diff --git a/lib/banzai/filter/abstract_reference_filter.rb b/lib/banzai/filter/abstract_reference_filter.rb
index 34c38913474..f21dbef216c 100644
--- a/lib/banzai/filter/abstract_reference_filter.rb
+++ b/lib/banzai/filter/abstract_reference_filter.rb
@@ -11,15 +11,19 @@ module Banzai
end
def self.object_name
- object_class.name.underscore
+ @object_name ||= object_class.name.underscore
end
def self.object_sym
- object_name.to_sym
+ @object_sym ||= object_name.to_sym
end
def self.data_reference
- "data-#{object_name.dasherize}"
+ @data_reference ||= "data-#{object_name.dasherize}"
+ end
+
+ def self.object_class_title
+ @object_title ||= object_class.name.titleize
end
# Public: Find references in text (like `!123` for merge requests)
@@ -53,6 +57,10 @@ module Banzai
self.class.object_sym
end
+ def object_class_title
+ self.class.object_class_title
+ end
+
def references_in(*args, &block)
self.class.references_in(*args, &block)
end
@@ -62,36 +70,81 @@ module Banzai
# Example: project.merge_requests.find
end
+ def find_object_cached(project, id)
+ if RequestStore.active?
+ cache = find_objects_cache[object_class][project.id]
+
+ get_or_set_cache(cache, id) { find_object(project, id) }
+ else
+ find_object(project, id)
+ end
+ end
+
+ def project_from_ref_cache(ref)
+ if RequestStore.active?
+ cache = project_refs_cache
+
+ get_or_set_cache(cache, ref) { project_from_ref(ref) }
+ else
+ project_from_ref(ref)
+ end
+ end
+
def url_for_object(object, project)
# Implement in child class
# Example: project_merge_request_url
end
- def call
- if object_class.reference_pattern
- # `#123`
- replace_text_nodes_matching(object_class.reference_pattern) do |content|
- object_link_filter(content, object_class.reference_pattern)
- end
+ def url_for_object_cached(object, project)
+ if RequestStore.active?
+ cache = url_for_object_cache[object_class][project.id]
- # `[Issue](#123)`, which is turned into
- # `<a href="#123">Issue</a>`
- replace_link_nodes_with_href(object_class.reference_pattern) do |link, text|
- object_link_filter(link, object_class.reference_pattern, link_text: text)
- end
+ get_or_set_cache(cache, object) { url_for_object(object, project) }
+ else
+ url_for_object(object, project)
end
+ end
- if object_class.link_reference_pattern
- # `http://gitlab.example.com/namespace/project/issues/123`, which is turned into
- # `<a href="http://gitlab.example.com/namespace/project/issues/123">http://gitlab.example.com/namespace/project/issues/123</a>`
- replace_link_nodes_with_text(object_class.link_reference_pattern) do |text|
- object_link_filter(text, object_class.link_reference_pattern)
- end
+ def call
+ return doc if project.nil?
+
+ ref_pattern = object_class.reference_pattern
+ link_pattern = object_class.link_reference_pattern
- # `[Issue](http://gitlab.example.com/namespace/project/issues/123)`, which is turned into
- # `<a href="http://gitlab.example.com/namespace/project/issues/123">Issue</a>`
- replace_link_nodes_with_href(object_class.link_reference_pattern) do |link, text|
- object_link_filter(link, object_class.link_reference_pattern, link_text: text)
+ each_node do |node|
+ if text_node?(node) && ref_pattern
+ replace_text_when_pattern_matches(node, ref_pattern) do |content|
+ object_link_filter(content, ref_pattern)
+ end
+
+ elsif element_node?(node)
+ yield_valid_link(node) do |link, text|
+ if ref_pattern && link =~ /\A#{ref_pattern}/
+ replace_link_node_with_href(node, link) do
+ object_link_filter(link, ref_pattern, link_text: text)
+ end
+
+ next
+ end
+
+ next unless link_pattern
+
+ if link == text && text =~ /\A#{link_pattern}/
+ replace_link_node_with_text(node, link) do
+ object_link_filter(text, link_pattern)
+ end
+
+ next
+ end
+
+ if link =~ /\A#{link_pattern}\z/
+ replace_link_node_with_href(node, link) do
+ object_link_filter(link, link_pattern, link_text: text)
+ end
+
+ next
+ end
+ end
end
end
@@ -109,9 +162,9 @@ module Banzai
# have `gfm` and `gfm-OBJECT_NAME` class names attached for styling.
def object_link_filter(text, pattern, link_text: nil)
references_in(text, pattern) do |match, id, project_ref, matches|
- project = project_from_ref(project_ref)
+ project = project_from_ref_cache(project_ref)
- if project && object = find_object(project, id)
+ if project && object = find_object_cached(project, id)
title = object_link_title(object)
klass = reference_class(object_sym)
@@ -121,8 +174,11 @@ module Banzai
object_sym => object.id
)
- url = matches[:url] if matches.names.include?("url")
- url ||= url_for_object(object, project)
+ if matches.names.include?("url") && matches[:url]
+ url = matches[:url]
+ else
+ url = url_for_object_cached(object, project)
+ end
text = link_text || object_link_text(object, matches)
@@ -146,7 +202,7 @@ module Banzai
end
def object_link_title(object)
- "#{object_class.name.titleize}: #{object.title}"
+ "#{object_class_title}: #{object.title}"
end
def object_link_text(object, matches)
@@ -157,6 +213,32 @@ module Banzai
text
end
+
+ private
+
+ def project_refs_cache
+ RequestStore[:banzai_project_refs] ||= {}
+ end
+
+ def find_objects_cache
+ RequestStore[:banzai_find_objects_cache] ||= Hash.new do |hash, key|
+ hash[key] = Hash.new { |h, k| h[k] = {} }
+ end
+ end
+
+ def url_for_object_cache
+ RequestStore[:banzai_url_for_object] ||= Hash.new do |hash, key|
+ hash[key] = Hash.new { |h, k| h[k] = {} }
+ end
+ end
+
+ def get_or_set_cache(cache, key)
+ if cache.key?(key)
+ cache[key]
+ else
+ cache[key] = yield
+ end
+ end
end
end
end
diff --git a/lib/banzai/filter/external_issue_reference_filter.rb b/lib/banzai/filter/external_issue_reference_filter.rb
index edc26386903..37344b90576 100644
--- a/lib/banzai/filter/external_issue_reference_filter.rb
+++ b/lib/banzai/filter/external_issue_reference_filter.rb
@@ -35,15 +35,29 @@ module Banzai
def call
# Early return if the project isn't using an external tracker
- return doc if project.nil? || project.default_issues_tracker?
+ return doc if project.nil? || default_issues_tracker?
- replace_text_nodes_matching(ExternalIssue.reference_pattern) do |content|
- issue_link_filter(content)
- end
+ ref_pattern = ExternalIssue.reference_pattern
+ ref_start_pattern = /\A#{ref_pattern}\z/
+
+ each_node do |node|
+ if text_node?(node)
+ replace_text_when_pattern_matches(node, ref_pattern) do |content|
+ issue_link_filter(content)
+ end
- replace_link_nodes_with_href(ExternalIssue.reference_pattern) do |link, text|
- issue_link_filter(link, link_text: text)
+ elsif element_node?(node)
+ yield_valid_link(node) do |link, text|
+ if link =~ ref_start_pattern
+ replace_link_node_with_href(node, link) do
+ issue_link_filter(link, link_text: text)
+ end
+ end
+ end
+ end
end
+
+ doc
end
# Replace `JIRA-123` issue references in text with links to the referenced
@@ -76,6 +90,21 @@ module Banzai
def url_for_issue(*args)
IssuesHelper.url_for_issue(*args)
end
+
+ def default_issues_tracker?
+ if RequestStore.active?
+ default_issues_tracker_cache[project.id] ||=
+ project.default_issues_tracker?
+ else
+ project.default_issues_tracker?
+ end
+ end
+
+ private
+
+ def default_issues_tracker_cache
+ RequestStore[:banzai_default_issues_tracker_cache] ||= {}
+ end
end
end
end
diff --git a/lib/banzai/filter/reference_filter.rb b/lib/banzai/filter/reference_filter.rb
index a3326ae042c..31386cf851c 100644
--- a/lib/banzai/filter/reference_filter.rb
+++ b/lib/banzai/filter/reference_filter.rb
@@ -52,18 +52,13 @@ module Banzai
html.html_safe? ? html : ERB::Util.html_escape_once(html)
end
- def ignore_parents
- @ignore_parents ||= begin
- # Don't look for references in text nodes that are children of these
- # elements.
+ def ignore_ancestor_query
+ @ignore_ancestor_query ||= begin
parents = %w(pre code a style)
parents << 'blockquote' if context[:ignore_blockquotes]
- parents.to_set
- end
- end
- def ignored_ancestry?(node)
- has_ancestor?(node, ignore_parents)
+ parents.map { |n| "ancestor::#{n}" }.join(' or ')
+ end
end
def project
@@ -74,119 +69,66 @@ module Banzai
"gfm gfm-#{type}"
end
- # Iterate through the document's text nodes, yielding the current node's
- # content if:
- #
- # * The `project` context value is present AND
- # * The node's content matches `pattern` AND
- # * The node is not an ancestor of an ignored node type
- #
- # pattern - Regex pattern against which to match the node's content
- #
- # Yields the current node's String contents. The result of the block will
- # replace the node's existing content and update the current document.
+ # Ensure that a :project key exists in context
#
- # Returns the updated Nokogiri::HTML::DocumentFragment object.
- def replace_text_nodes_matching(pattern)
- return doc if project.nil?
-
- search_text_nodes(doc).each do |node|
- next if ignored_ancestry?(node)
- next unless node.text =~ pattern
-
- content = node.to_html
-
- html = yield content
-
- next if html == content
-
- node.replace(html)
- end
-
- doc
+ # Note that while the key might exist, its value could be nil!
+ def validate
+ needs :project
end
- # Iterate through the document's link nodes, yielding the current node's
- # content if:
- #
- # * The `project` context value is present AND
- # * The node's content matches `pattern`
- #
- # pattern - Regex pattern against which to match the node's content
- #
- # Yields the current node's String contents. The result of the block will
- # replace the node and update the current document.
+ # Iterates over all <a> and text() nodes in a document.
#
- # Returns the updated Nokogiri::HTML::DocumentFragment object.
- def replace_link_nodes_with_text(pattern)
- return doc if project.nil?
+ # Nodes are skipped whenever their ancestor is one of the nodes returned
+ # by `ignore_ancestor_query`. Link tags are not processed if they have a
+ # "gfm" class or the "href" attribute is empty.
+ def each_node
+ query = %Q{descendant-or-self::text()[not(#{ignore_ancestor_query})]
+ | descendant-or-self::a[
+ not(contains(concat(" ", @class, " "), " gfm ")) and not(@href = "")
+ ]}
- doc.xpath('descendant-or-self::a').each do |node|
- klass = node.attr('class')
- next if klass && klass.include?('gfm')
-
- link = node.attr('href')
- text = node.text
-
- next unless link && text
-
- link = CGI.unescape(link)
- next unless link.force_encoding('UTF-8').valid_encoding?
- # Ignore ending punctionation like periods or commas
- next unless link == text && text =~ /\A#{pattern}/
-
- html = yield text
+ doc.xpath(query).each do |node|
+ yield node
+ end
+ end
- next if html == text
+ # Yields the link's URL and text whenever the node is a valid <a> tag.
+ def yield_valid_link(node)
+ link = CGI.unescape(node.attr('href').to_s)
+ text = node.text
- node.replace(html)
- end
+ return unless link.force_encoding('UTF-8').valid_encoding?
- doc
+ yield link, text
end
- # Iterate through the document's link nodes, yielding the current node's
- # content if:
- #
- # * The `project` context value is present AND
- # * The node's HREF matches `pattern`
- #
- # pattern - Regex pattern against which to match the node's HREF
- #
- # Yields the current node's String HREF and String content.
- # The result of the block will replace the node and update the current document.
- #
- # Returns the updated Nokogiri::HTML::DocumentFragment object.
- def replace_link_nodes_with_href(pattern)
- return doc if project.nil?
+ def replace_text_when_pattern_matches(node, pattern)
+ return unless node.text =~ pattern
- doc.xpath('descendant-or-self::a').each do |node|
- klass = node.attr('class')
- next if klass && klass.include?('gfm')
+ content = node.to_html
+ html = yield content
- link = node.attr('href')
- text = node.text
+ node.replace(html) unless content == html
+ end
- next unless link && text
- link = CGI.unescape(link)
- next unless link.force_encoding('UTF-8').valid_encoding?
- next unless link && link =~ /\A#{pattern}\z/
+ def replace_link_node_with_text(node, link)
+ html = yield
- html = yield link, text
+ node.replace(html) unless html == node.text
+ end
- next if html == link
+ def replace_link_node_with_href(node, link)
+ html = yield
- node.replace(html)
- end
+ node.replace(html) unless html == link
+ end
- doc
+ def text_node?(node)
+ node.is_a?(Nokogiri::XML::Text)
end
- # Ensure that a :project key exists in context
- #
- # Note that while the key might exist, its value could be nil!
- def validate
- needs :project
+ def element_node?(node)
+ node.is_a?(Nokogiri::XML::Element)
end
end
end
diff --git a/lib/banzai/filter/user_reference_filter.rb b/lib/banzai/filter/user_reference_filter.rb
index 989fa64e078..eea3af842b6 100644
--- a/lib/banzai/filter/user_reference_filter.rb
+++ b/lib/banzai/filter/user_reference_filter.rb
@@ -59,13 +59,28 @@ module Banzai
end
def call
- replace_text_nodes_matching(User.reference_pattern) do |content|
- user_link_filter(content)
+ return doc if project.nil?
+
+ ref_pattern = User.reference_pattern
+ ref_pattern_start = /\A#{ref_pattern}\z/
+
+ each_node do |node|
+ if text_node?(node)
+ replace_text_when_pattern_matches(node, ref_pattern) do |content|
+ user_link_filter(content)
+ end
+ elsif element_node?(node)
+ yield_valid_link(node) do |link, text|
+ if link =~ ref_pattern_start
+ replace_link_node_with_href(node, link) do
+ user_link_filter(link, link_text: text)
+ end
+ end
+ end
+ end
end
- replace_link_nodes_with_href(User.reference_pattern) do |link, text|
- user_link_filter(link, link_text: text)
- end
+ doc
end
# Replace `@user` user references in text with links to the referenced