From b02458ef52ac3e61d3c8b924e092e956375c15f7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 27 Apr 2019 22:25:45 -0700 Subject: Fix slow performance with compiling HAML templates In Rails 5, including `ActionView::Context` can have a significant and hidden performance penalty because this module also includes `ActionView::CompiledTemplates`. This means that any module that includes ActionView::Context becomes a descendant of `CompiledTemplates`. When a partial is rendered for the first time, it runs `ActionView::CompiledTemplates#module_eval`, which will evaluate a string that defines a new method for that partial. For example, the source of partial might be this string: ``` def _app_views_project_show_html_haml___12345(local_assigns, output) "hello world" end ``` When this string is evaluated, the Ruby interpreter will define the method and clear the global method cache for all descendants of `ActionView::CompiledTemplates`. Previous to this change, we inadvertently made a number of modules fall into this category: * GroupChildEntity * NoteUserEntity * Notify * MergeRequestUserEntity * AnalyticsCommitEntity * CommitEntity * UserEntity * Kaminari::Helpers::Paginator * CurrentUserEntity * ActionView::Base * ActionDispatch::DebugExceptions::DebugView * MarkupHelper * MergeRequestPresenter After this change: * Kaminari::Helpers::Paginator * ActionView::Base * ActionDispatch::DebugExceptions::DebugView Each time a partial is rendered for the first time, all methods for those modules will have to be redefined. This can exact a significant performance penalty. How bad is this penalty? Using the following benchmark script, we can use DTrace to sample the Ruby interpreter: ``` Benchmark.bm do |x| x.report do 1000.times do ActionView::CompiledTemplates.module_eval("def testme\nend") end end end ``` This revealed a 11x jump in the time spent in `core#define_method` alone. Rails 6 fixes this behavior by moving the `include CompiledTemplates` into ActionView::Base so that including `ActionView::Context` doesn't quietly affect other modules in this way. Closes https://gitlab.com/gitlab-org/gitlab-ee/issues/11198 --- app/helpers/markup_helper.rb | 2 +- app/serializers/concerns/user_status_tooltip.rb | 2 +- .../unreleased/sh-fix-slow-partial-rendering.yml | 5 +++ lib/gitlab/action_view_output/context.rb | 41 ++++++++++++++++++++++ 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-slow-partial-rendering.yml create mode 100644 lib/gitlab/action_view_output/context.rb diff --git a/app/helpers/markup_helper.rb b/app/helpers/markup_helper.rb index ad77f99fe44..dce4168ad7b 100644 --- a/app/helpers/markup_helper.rb +++ b/app/helpers/markup_helper.rb @@ -4,7 +4,7 @@ require 'nokogiri' module MarkupHelper include ActionView::Helpers::TagHelper - include ActionView::Context + include ::Gitlab::ActionViewOutput::Context def plain?(filename) Gitlab::MarkupHelper.plain?(filename) diff --git a/app/serializers/concerns/user_status_tooltip.rb b/app/serializers/concerns/user_status_tooltip.rb index 633b117d392..a81e377691e 100644 --- a/app/serializers/concerns/user_status_tooltip.rb +++ b/app/serializers/concerns/user_status_tooltip.rb @@ -3,7 +3,7 @@ module UserStatusTooltip extend ActiveSupport::Concern include ActionView::Helpers::TagHelper - include ActionView::Context + include ::Gitlab::ActionViewOutput::Context include EmojiHelper include UsersHelper diff --git a/changelogs/unreleased/sh-fix-slow-partial-rendering.yml b/changelogs/unreleased/sh-fix-slow-partial-rendering.yml new file mode 100644 index 00000000000..0f65a6f8d69 --- /dev/null +++ b/changelogs/unreleased/sh-fix-slow-partial-rendering.yml @@ -0,0 +1,5 @@ +--- +title: Fix slow performance with compiling HAML templates +merge_request: 27782 +author: +type: performance diff --git a/lib/gitlab/action_view_output/context.rb b/lib/gitlab/action_view_output/context.rb new file mode 100644 index 00000000000..9fbc9811636 --- /dev/null +++ b/lib/gitlab/action_view_output/context.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +# This file was simplified from https://raw.githubusercontent.com/rails/rails/195f39804a7a4a0034f25e8704220e03d95a752a/actionview/lib/action_view/context.rb. +# +# It is only needed by modules that need to call ActionView helper +# methods (e.g. those in +# https://github.com/rails/rails/tree/c4d3e202e10ae627b3b9c34498afb45450652421/actionview/lib/action_view/helpers) +# to generate tags outside of a Rails controller (e.g. API, Sidekiq, +# etc.). +# +# In Rails 5, ActionView::Context automatically includes CompiledTemplates. +# This means that any module that includes ActionView::Context is now a descendant +# of CompiledTemplates. +# +# When a partial is rendered for the first time, it runs +# Module#module_eval, which will evaluate a string source that defines a +# new method. For example: +# +# def _app_views_profiles_show_html_haml___1285955918103175884_70307801785400(local_assigns, output_buffer) +# "hello world" +# end +# +# When a new method is defined, the Ruby interpreter clears the method +# cache for all descendants, and all methods for those modules will have +# to be redefined. This can lead to a significant performance penalty. +# +# Rails 6 fixes this behavior by moving out the `include +# CompiledTemplates` into ActionView::Base so that including `ActionView::Context` +# doesn't quietly affect other modules in this way. + +if Rails::VERSION::STRING.start_with?('6') + raise 'This module is no longer needed in Rails 6. Use ActionView::Context instead.' +end + +module Gitlab + module ActionViewOutput + module Context + attr_accessor :output_buffer, :view_flow + end + end +end -- cgit v1.2.1 From fad99d934f73536929c4a12e25308473b52769b5 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 27 Apr 2019 23:38:05 -0700 Subject: Add Rubocop rule to ban include ActionView::Context --- rubocop/cop/include_action_view_context.rb | 31 +++++++++++++++ rubocop/rubocop.rb | 1 + .../cop/include_action_view_context_spec.rb | 45 ++++++++++++++++++++++ 3 files changed, 77 insertions(+) create mode 100644 rubocop/cop/include_action_view_context.rb create mode 100644 spec/rubocop/cop/include_action_view_context_spec.rb diff --git a/rubocop/cop/include_action_view_context.rb b/rubocop/cop/include_action_view_context.rb new file mode 100644 index 00000000000..14662a33e95 --- /dev/null +++ b/rubocop/cop/include_action_view_context.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require_relative '../spec_helpers' + +module RuboCop + module Cop + # Cop that makes sure workers include `::Gitlab::ActionViewOutput::Context`, not `ActionView::Context`. + class IncludeActionViewContext < RuboCop::Cop::Cop + include SpecHelpers + + MSG = 'Include `::Gitlab::ActionViewOutput::Context`, not `ActionView::Context`, for Rails 5.'.freeze + + def_node_matcher :includes_action_view_context?, <<~PATTERN + (send nil? :include (const (const nil? :ActionView) :Context)) + PATTERN + + def on_send(node) + return if in_spec?(node) + return unless includes_action_view_context?(node) + + add_offense(node.arguments.first, location: :expression) + end + + def autocorrect(node) + lambda do |corrector| + corrector.replace(node.source_range, '::Gitlab::ActionViewOutput::Context') + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 50eab6f9270..ce6bdbf292c 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -4,6 +4,7 @@ require_relative 'cop/gitlab/predicate_memoization' require_relative 'cop/gitlab/httparty' require_relative 'cop/gitlab/finder_with_find_by' require_relative 'cop/gitlab/union' +require_relative 'cop/include_action_view_context' require_relative 'cop/include_sidekiq_worker' require_relative 'cop/safe_params' require_relative 'cop/active_record_association_reload' diff --git a/spec/rubocop/cop/include_action_view_context_spec.rb b/spec/rubocop/cop/include_action_view_context_spec.rb new file mode 100644 index 00000000000..c888555b54f --- /dev/null +++ b/spec/rubocop/cop/include_action_view_context_spec.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../rubocop/cop/include_action_view_context' + +describe RuboCop::Cop::IncludeActionViewContext do + include CopHelper + + subject(:cop) { described_class.new } + + context 'when `ActionView::Context` is included' do + let(:source) { 'include ActionView::Context' } + let(:correct_source) { 'include ::Gitlab::ActionViewOutput::Context' } + + it 'registers an offense' do + inspect_source(source) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([1]) + expect(cop.highlights).to eq(['ActionView::Context']) + end + end + + it 'autocorrects to the right version' do + autocorrected = autocorrect_source(source) + + expect(autocorrected).to eq(correct_source) + end + end + + context 'when `ActionView::Context` is not included' do + it 'registers no offense' do + inspect_source('include Context') + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + end +end -- cgit v1.2.1