Merge branch 'leipert-improve-ansi2html' into 'master'

Improve job log rendering performance

See merge request gitlab-org/gitlab-ce!31262

7 files changed, 54 insertions, 44 deletions

diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss
index 6e98908eeed..262c0bf5ed2 100644
--- a/app/assets/stylesheets/pages/builds.scss
+++ b/app/assets/stylesheets/pages/builds.scss
@@ -127,6 +127,7 @@
 
   .section-header ~ .section.line {
     margin-left: $gl-padding;
+    display: block;
   }
 }
 
diff --git a/changelogs/unreleased/leipert-improve-ansi2html.yml b/changelogs/unreleased/leipert-improve-ansi2html.yml
new file mode 100644
index 00000000000..dd3582b3434
--- /dev/null
+++ b/changelogs/unreleased/leipert-improve-ansi2html.yml
@@ -0,0 +1,5 @@
+---
+title: Improve job log rendering performance
+merge_request: 31262
+author:
+type: performance
diff --git a/lib/gitlab/ci/ansi2html.rb b/lib/gitlab/ci/ansi2html.rb
index fc3223e7442..7e348763e81 100644
--- a/lib/gitlab/ci/ansi2html.rb
+++ b/lib/gitlab/ci/ansi2html.rb
@@ -194,16 +194,10 @@ module Gitlab
         end
 
         def handle_new_line
-          css_classes = []
-
-          if @sections.any?
-            css_classes = %w[section line] + sections.map { |section| "s_#{section}" }
-          end
-
           write_in_tag %{<br/>}
-          write_raw %{<span class="#{css_classes.join(' ')}"></span>} if css_classes.any?
+
+          close_open_tags if @sections.any? && @lineno_in_section == 0
           @lineno_in_section += 1
-          open_new_tag
         end
 
         def handle_section(scanner)
@@ -310,11 +304,24 @@ module Gitlab
 
           if @sections.any?
             css_classes << "section"
-            css_classes << "js-section-header section-header" if @lineno_in_section == 0
+
+            css_classes << if @lineno_in_section == 0
+                             "js-section-header section-header"
+                           else
+                             "line"
+                           end
+
             css_classes += sections.map { |section| "js-s-#{section}" }
           end
 
-          @out << %{<span class="#{css_classes.join(' ')}">}
+          close_open_tags
+
+          @out << if css_classes.any?
+                    %{<span class="#{css_classes.join(' ')}">}
+                  else
+                    %{<span>}
+                  end
+
           @n_open_tags += 1
         end
 
diff --git a/spec/controllers/projects/jobs_controller_spec.rb b/spec/controllers/projects/jobs_controller_spec.rb
index 39ebf02dcf5..f076a5e769f 100644
--- a/spec/controllers/projects/jobs_controller_spec.rb
+++ b/spec/controllers/projects/jobs_controller_spec.rb
@@ -546,7 +546,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response['id']).to eq job.id
         expect(json_response['status']).to eq job.status
-        expect(json_response['html']).to eq('<span class="">BUILD TRACE</span>')
+        expect(json_response['html']).to eq('<span>BUILD TRACE</span>')
       end
     end
 
diff --git a/spec/lib/gitlab/ci/ansi2html_spec.rb b/spec/lib/gitlab/ci/ansi2html_spec.rb
index 651fdaaabca..eaf06ed8992 100644
--- a/spec/lib/gitlab/ci/ansi2html_spec.rb
+++ b/spec/lib/gitlab/ci/ansi2html_spec.rb
@@ -6,11 +6,11 @@ describe Gitlab::Ci::Ansi2html do
   subject { described_class }
 
   it "prints non-ansi as-is" do
-    expect(convert_html("Hello")).to eq('<span class="">Hello</span>')
+    expect(convert_html("Hello")).to eq('<span>Hello</span>')
   end
 
   it "strips non-color-changing control sequences" do
-    expect(convert_html("Hello \e[2Kworld")).to eq('<span class="">Hello world</span>')
+    expect(convert_html("Hello \e[2Kworld")).to eq('<span>Hello world</span>')
   end
 
   it "prints simply red" do
@@ -34,7 +34,7 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets colors after red on blue" do
-    expect(convert_html("\e[31;44mHello\e[0m world")).to eq('<span class="term-fg-red term-bg-blue">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[31;44mHello\e[0m world")).to eq('<span class="term-fg-red term-bg-blue">Hello</span><span> world</span>')
   end
 
   it "performs color change from red/blue to yellow/blue" do
@@ -46,11 +46,11 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "performs color change from red/blue to reset to yellow/green" do
-    expect(convert_html("\e[31;44mHello\e[0m \e[33;42mworld")).to eq('<span class="term-fg-red term-bg-blue">Hello</span><span class=""> </span><span class="term-fg-yellow term-bg-green">world</span>')
+    expect(convert_html("\e[31;44mHello\e[0m \e[33;42mworld")).to eq('<span class="term-fg-red term-bg-blue">Hello</span><span> </span><span class="term-fg-yellow term-bg-green">world</span>')
   end
 
   it "ignores unsupported codes" do
-    expect(convert_html("\e[51mHello\e[0m")).to eq('<span class="">Hello</span>')
+    expect(convert_html("\e[51mHello\e[0m")).to eq('<span>Hello</span>')
   end
 
   it "prints light red" do
@@ -74,8 +74,8 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets bold text" do
-    expect(convert_html("\e[1mHello\e[21m world")).to eq('<span class="term-bold">Hello</span><span class=""> world</span>')
-    expect(convert_html("\e[1mHello\e[22m world")).to eq('<span class="term-bold">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[1mHello\e[21m world")).to eq('<span class="term-bold">Hello</span><span> world</span>')
+    expect(convert_html("\e[1mHello\e[22m world")).to eq('<span class="term-bold">Hello</span><span> world</span>')
   end
 
   it "prints italic text" do
@@ -83,7 +83,7 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets italic text" do
-    expect(convert_html("\e[3mHello\e[23m world")).to eq('<span class="term-italic">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[3mHello\e[23m world")).to eq('<span class="term-italic">Hello</span><span> world</span>')
   end
 
   it "prints underlined text" do
@@ -91,7 +91,7 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets underlined text" do
-    expect(convert_html("\e[4mHello\e[24m world")).to eq('<span class="term-underline">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[4mHello\e[24m world")).to eq('<span class="term-underline">Hello</span><span> world</span>')
   end
 
   it "prints concealed text" do
@@ -99,7 +99,7 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets concealed text" do
-    expect(convert_html("\e[8mHello\e[28m world")).to eq('<span class="term-conceal">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[8mHello\e[28m world")).to eq('<span class="term-conceal">Hello</span><span> world</span>')
   end
 
   it "prints crossed-out text" do
@@ -107,7 +107,7 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "resets crossed-out text" do
-    expect(convert_html("\e[9mHello\e[29m world")).to eq('<span class="term-cross">Hello</span><span class=""> world</span>')
+    expect(convert_html("\e[9mHello\e[29m world")).to eq('<span class="term-cross">Hello</span><span> world</span>')
   end
 
   it "can print 256 xterm fg colors" do
@@ -139,15 +139,15 @@ describe Gitlab::Ci::Ansi2html do
   end
 
   it "prints &lt;" do
-    expect(convert_html("<")).to eq('<span class="">&lt;</span>')
+    expect(convert_html("<")).to eq('<span>&lt;</span>')
   end
 
   it "replaces newlines with line break tags" do
-    expect(convert_html("\n")).to eq('<span class=""><br/><span class=""></span></span>')
+    expect(convert_html("\n")).to eq('<span><br/></span>')
   end
 
   it "groups carriage returns with newlines" do
-    expect(convert_html("\r\n")).to eq('<span class=""><br/><span class=""></span></span>')
+    expect(convert_html("\r\n")).to eq('<span><br/></span>')
   end
 
   describe "incremental update" do
@@ -184,7 +184,7 @@ describe Gitlab::Ci::Ansi2html do
 
     context "with partial sequence" do
       let(:pre_text) { "Hello\e" }
-      let(:pre_html) { "<span class=\"\">Hello</span>" }
+      let(:pre_html) { "<span>Hello</span>" }
       let(:text) { "[1m World" }
       let(:html) { "<span class=\"term-bold\"> World</span>" }
 
@@ -193,9 +193,9 @@ describe Gitlab::Ci::Ansi2html do
 
     context 'with new line' do
       let(:pre_text) { "Hello\r" }
-      let(:pre_html) { "<span class=\"\">Hello\r</span>" }
+      let(:pre_html) { "<span>Hello\r</span>" }
       let(:text) { "\nWorld" }
-      let(:html) { "<span class=\"\"><br/><span class=\"\">World</span></span>" }
+      let(:html) { "<span><br/>World</span>" }
 
       it_behaves_like 'stateable converter'
     end
@@ -222,7 +222,7 @@ describe Gitlab::Ci::Ansi2html do
         text = "#{section_start}Some text#{section_end}"
         class_name_start = section_start.gsub("\033[0K", '').gsub('<', '&lt;')
         class_name_end = section_end.gsub("\033[0K", '').gsub('<', '&lt;')
-        html = %{<span class="">#{class_name_start}Some text#{class_name_end}</span>}
+        html = %{<span>#{class_name_start}Some text#{class_name_end}</span>}
 
         expect(convert_html(text)).to eq(html)
       end
@@ -232,12 +232,11 @@ describe Gitlab::Ci::Ansi2html do
       let(:text) { "#{section_start}Some text#{section_end}" }
 
       it 'prints light red' do
-        text = "#{section_start}\e[91mHello\e[0m\n#{section_end}"
+        text = "#{section_start}\e[91mHello\e[0m\nLine 1\nLine 2\nLine 3\n#{section_end}"
         header = %{<span class="term-fg-l-red section js-section-header section-header js-s-#{class_name(section_name)}">Hello</span>}
         line_break = %{<span class="section js-section-header section-header js-s-#{class_name(section_name)}"><br/></span>}
-        line = %{<span class="section line s_#{class_name(section_name)}"></span>}
-        empty_line = %{<span class="section js-s-#{class_name(section_name)}"></span>}
-        html = "#{section_start_html}#{header}#{line_break}#{line}#{empty_line}#{section_end_html}"
+        output_line = %{<span class="section line js-s-#{class_name(section_name)}">Line 1<br/>Line 2<br/>Line 3<br/></span>}
+        html = "#{section_start_html}#{header}#{line_break}#{output_line}#{section_end_html}"
 
         expect(convert_html(text)).to eq(html)
       end
diff --git a/spec/lib/gitlab/ci/trace/stream_spec.rb b/spec/lib/gitlab/ci/trace/stream_spec.rb
index 35250632e86..af519f4bae6 100644
--- a/spec/lib/gitlab/ci/trace/stream_spec.rb
+++ b/spec/lib/gitlab/ci/trace/stream_spec.rb
@@ -65,9 +65,9 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
           result = stream.html
 
           expect(result).to eq(
-            "<span class=\"\">ヾ(´༎ຶД༎ຶ`)ﾉ<br/><span class=\"\"></span></span>"\
-            "<span class=\"term-fg-green\">許功蓋</span><span class=\"\"><br/>"\
-            "<span class=\"\"></span></span>")
+            "<span>ヾ(´༎ຶД༎ຶ`)ﾉ<br/></span>"\
+            "<span class=\"term-fg-green\">許功蓋</span>"\
+            "<span><br/></span>")
           expect(result.encoding).to eq(Encoding.default_external)
         end
       end
@@ -253,7 +253,7 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
       it 'returns html content with state' do
         result = stream.html_with_state
 
-        expect(result.html).to eq("<span class=\"\">1234</span>")
+        expect(result.html).to eq("<span>1234</span>")
       end
 
       context 'follow-up state' do
@@ -269,7 +269,7 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
           result = stream.html_with_state(last_result.state)
 
           expect(result.append).to be_truthy
-          expect(result.html).to eq("<span class=\"\">5678</span>")
+          expect(result.html).to eq("<span>5678</span>")
         end
       end
     end
@@ -305,13 +305,11 @@ describe Gitlab::Ci::Trace::Stream, :clean_gitlab_redis_cache do
   describe '#html' do
     shared_examples_for 'htmls' do
       it "returns html" do
-        expect(stream.html).to eq(
-          "<span class=\"\">12<br/><span class=\"\">34<br/>"\
-          "<span class=\"\">56</span></span></span>")
+        expect(stream.html).to eq("<span>12<br/>34<br/>56</span>")
       end
 
       it "returns html for last line only" do
-        expect(stream.html(last_lines: 1)).to eq("<span class=\"\">56</span>")
+        expect(stream.html(last_lines: 1)).to eq("<span>56</span>")
       end
     end
 
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index 6fd4b14d51d..e2b4b50d41d 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/spec/support/shared_examples/ci_trace_shared_examples.rb
@@ -7,11 +7,11 @@ shared_examples_for 'common trace features' do
     end
 
     it "returns formatted html" do
-      expect(trace.html).to eq("<span class=\"\">12<br/><span class=\"\">34</span></span>")
+      expect(trace.html).to eq("<span>12<br/>34</span>")
     end
 
     it "returns last line of formatted html" do
-      expect(trace.html(last_lines: 1)).to eq("<span class=\"\">34</span>")
+      expect(trace.html(last_lines: 1)).to eq("<span>34</span>")
     end
   end