diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-04-04 10:14:56 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-04-04 10:14:56 +0000 |
commit | 4bf302799f7bd32e27c0fe83abe34239d9c3e1fd (patch) | |
tree | df691dca4b9768ffc5b2ab66ca1571fb59e18510 | |
parent | 3dd91f55a048c13e6dc81ec685e521d2fc5a78f5 (diff) | |
parent | 915fd3f910921bc97abdda5f2ae63093c11533fd (diff) | |
download | gitlab-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-- | CHANGELOG | 1 | ||||
-rw-r--r-- | Gemfile | 2 | ||||
-rw-r--r-- | Gemfile.lock | 4 | ||||
-rw-r--r-- | lib/banzai/filter/abstract_reference_filter.rb | 140 | ||||
-rw-r--r-- | lib/banzai/filter/external_issue_reference_filter.rb | 41 | ||||
-rw-r--r-- | lib/banzai/filter/reference_filter.rb | 148 | ||||
-rw-r--r-- | lib/banzai/filter/user_reference_filter.rb | 25 |
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) @@ -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 |