Merge pull request #311 from bouk/fix-quadratic-performance-in-concat…

…_javascript_sources

Fix quadratic performance in concat_javascript_sources

lib/sprockets/utils.rb

@@ -76,16 +76,19 @@ def hash_reassoc(hash, key_a, key_b = nil, &block)
     def string_end_with_semicolon?(str)
       i = str.size - 1
       while i >= 0
-        c = str[i]
+        c = str[i].ord
         i -= 1
-        if c == "\n" || c == " " || c == "\t"
-          next
-        elsif c != ";"
-          return false
-        else
-          return true
+
+        # Need to compare against the ordinals because the string can be UTF_8 or UTF_32LE encoded
+        # 0x0A == "\n"
+        # 0x20 == " "
+        # 0x09 == "\t"
+        # 0x3B == ";"
+        unless c == 0x0A || c == 0x20 || c == 0x09
+          return c === 0x3B
         end
       end
+
       true
     end
 
@@ -97,11 +100,21 @@ def string_end_with_semicolon?(str)
     #
     # Returns buf String.
     def concat_javascript_sources(buf, source)
-      if buf.bytesize > 0
-        buf << ";" unless string_end_with_semicolon?(buf)
-        buf << "\n" unless buf.end_with?("\n")
+      if source.bytesize > 0
+        buf << source
+
+        # If the source contains non-ASCII characters, indexing on it becomes O(N).
+        # This will lead to O(N^2) performance in string_end_with_semicolon?, so we should use 32 bit encoding to make sure indexing stays O(1)
+        source = source.encode(Encoding::UTF_32LE) unless source.ascii_only?
+
+        if !string_end_with_semicolon?(source)
+          buf << ";\n"
+        elsif source[source.size - 1].ord != 0x0A
+          buf << "\n"
+        end
       end
-      buf << source
+
+      buf
     end
 
     # Internal: Inject into target module for the duration of the block.

test/fixtures/context/resolve_content_type.js.erb

@@ -1,2 +1,2 @@
-<%= resolve("foo.js") %>,
-<%= resolve("foo", accept: "application/javascript") %>
+<%= resolve("foo.js") %>;
+<%= resolve("foo", accept: "application/javascript") %>;

test/test_asset.rb

@@ -878,6 +878,7 @@ def setup
 define("application.js", "application-955b2dddd0d1449b1c617124b83b46300edadec06d561104f7f6165241b31a94.js")
 define("application.css", "application-46d50149c56fc370805f53c29f79b89a52d4cc479eeebcdc8db84ab116d7ab1a.css")
 define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png")
+;
     EOS
     assert_equal [
       "file://#{fixture_path_for_uri("asset/POW.png")}?type=image/png&id=xxx",
@@ -896,6 +897,7 @@ def setup
 define("application.js", "application-955b2dddd0d1449b1c617124b83b46300edadec06d561104f7f6165241b31a94.js")
 define("application.css", "application-46d50149c56fc370805f53c29f79b89a52d4cc479eeebcdc8db84ab116d7ab1a.css")
 define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png")
+;
     EOS
 
     assert_equal [
@@ -1050,12 +1052,12 @@ def setup
   end
 
   test "appends missing semicolons" do
-    assert_equal "var Bar\n;\n\n(function() {\n  var Foo\n})\n",
+    assert_equal "var Bar\n;\n\n(function() {\n  var Foo\n})\n;\n",
       asset("semicolons.js").to_s
   end
 
   test 'keeps code in same line after multi-line comments' do
-    assert_equal "/******/ function foo() {\n}\n",
+    assert_equal "/******/ function foo() {\n}\n;\n",
       asset('multi_line_comment.js').to_s
   end
 

test/test_context.rb

@@ -50,10 +50,10 @@ def datauri(path)
   end
 
   test "resolve with content type" do
-    assert_equal [
-      "file://#{fixture_path_for_uri("context/foo.js")}?type=application/javascript",
-      "file://#{fixture_path_for_uri("context/foo.js")}?type=application/javascript"
-    ].join(",\n"), @env["resolve_content_type.js"].to_s.strip
+    assert_equal(<<-FILE, @env["resolve_content_type.js"].to_s)
+file://#{fixture_path_for_uri("context/foo.js")}?type=application/javascript;
+file://#{fixture_path_for_uri("context/foo.js")}?type=application/javascript;
+FILE
   end
 end
 

test/test_environment.rb

@@ -744,7 +744,7 @@ def foo; end
     assert processor = @env.preprocessors['application/javascript'][0]
     assert_kind_of Sprockets::DirectiveProcessor, processor
     @env.unregister_preprocessor('application/javascript', processor)
-    assert_equal "// =require \"notfound\"\n", @env["missing_require.js"].to_s
+    assert_equal "// =require \"notfound\"\n;\n", @env["missing_require.js"].to_s
   end
 end
 

test/test_utils.rb

@@ -90,16 +90,21 @@ def test_string_ends_with_semicolon
   end
 
   def test_concat_javascript_sources
-    assert_equal "var foo;\n", concat_javascript_sources(String.new(""), "var foo;\n".freeze)
-    assert_equal "\nvar foo;\n", concat_javascript_sources(String.new("\n"), "var foo;\n".freeze)
-    assert_equal " \nvar foo;\n", concat_javascript_sources(String.new(" "), "var foo;\n".freeze)
-
-    assert_equal "var foo;\nvar bar;\n", concat_javascript_sources(String.new("var foo;\n"), "var bar;\n".freeze)
-    assert_equal "var foo;\nvar bar", concat_javascript_sources(String.new("var foo"), "var bar".freeze)
-    assert_equal "var foo;\nvar bar;", concat_javascript_sources(String.new("var foo;"), "var bar;".freeze)
-    assert_equal "var foo;\nvar bar;", concat_javascript_sources(String.new("var foo"), "var bar;".freeze)
+    assert_equal "var foo;\n", apply_concat_javascript_sources("".freeze, "var foo;\n".freeze)
+    assert_equal "\nvar foo;\n", apply_concat_javascript_sources("\n".freeze, "var foo;\n".freeze)
+    assert_equal " \nvar foo;\n", apply_concat_javascript_sources(" ".freeze, "var foo;\n".freeze)
+
+    assert_equal "var foo;\nvar bar;\n", apply_concat_javascript_sources("var foo;\n".freeze, "var bar;\n".freeze)
+    assert_equal "var foo;\nvar bar;\n", apply_concat_javascript_sources("var foo".freeze, "var bar".freeze)
+    assert_equal "var foo;\nvar bar;\n", apply_concat_javascript_sources("var foo;".freeze, "var bar;".freeze)
+    assert_equal "var foo;\nvar bar;\n", apply_concat_javascript_sources("var foo".freeze, "var bar;".freeze)
   end
 
+  def apply_concat_javascript_sources(*args)
+    args.reduce(String.new(""), &method(:concat_javascript_sources))
+  end
+
+
   def test_post_order_depth_first_search
     m = []
     m[11] = [4, 5, 10]