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

Fix quadratic performance in concat_javascript_sources

Author: rafaelfranca

lib/sprockets/utils.rb

@@ -74,16 +74,19 @@ def hash_reassoc(hash, *keys, &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
 
@@ -95,11 +98,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: Prepends a leading "." to an extension if its missing.

test/fixtures/context/resolve_content_type.js.erb

@@ -1,3 +1,3 @@
-<%= resolve("foo.js", compat: true) %>,
-<%= resolve("foo.js", :content_type => "application/javascript", compat: true) %>,
+<%= resolve("foo.js", compat: true) %>;
+<%= resolve("foo.js", :content_type => "application/javascript", compat: true) %>;
 <%= resolve("foo.js", :content_type => :self, compat: true) %>

test/test_asset.rb

@@ -125,26 +125,6 @@ def self.test(name, &block)
     end
   end
 
-  test "modify asset's dependency file" do
-    main = fixture_path('asset/test-main.js.erb')
-    dep  = fixture_path('asset/test-dep.js')
-
-    sandbox main, dep do
-      write(main, "//= depend_on test-dep\n<%= File.read('#{dep}') %>")
-      write(dep, "a;")
-      asset      = asset('test-main.js')
-      old_digest = asset.hexdigest
-      old_uri    = asset.uri
-      assert_equal "a;", asset.to_s
-
-      write(dep, "b;")
-      asset = asset('test-main.js')
-      refute_equal old_digest, asset.hexdigest
-      refute_equal old_uri, asset.uri
-      assert_equal "b;", asset.to_s
-    end
-  end
-
   test "remove asset's dependency file" do
     main = fixture_path('asset/test-main.js')
     dep  = fixture_path('asset/test-dep.js')
@@ -420,6 +400,26 @@ def setup
 
   include AssetTests
 
+  test "modify asset's dependency file" do
+    main = fixture_path('asset/test-main.js.erb')
+    dep  = fixture_path('asset/test-dep.js')
+
+    sandbox main, dep do
+      write(main, "//= depend_on test-dep\n<%= File.read('#{dep}') %>")
+      write(dep, "a;")
+      asset      = asset('test-main.js')
+      old_digest = asset.hexdigest
+      old_uri    = asset.uri
+      assert_equal "a;", asset.to_s
+
+      write(dep, "b;")
+      asset = asset('test-main.js')
+      refute_equal old_digest, asset.hexdigest
+      refute_equal old_uri, asset.uri
+      assert_equal "b;", asset.to_s
+    end
+  end
+
   test "uri" do
     assert_equal "file://#{fixture_path_for_uri('asset/application.js')}?type=application/javascript&pipeline=self&id=xxx",
       normalize_uri(@asset.uri)
@@ -508,6 +508,27 @@ def setup
 
   include AssetTests
 
+  test "modify asset's dependency file" do
+    main = fixture_path('asset/test-main.js.erb')
+    dep  = fixture_path('asset/test-dep.js')
+
+    sandbox main, dep do
+      write(main, "//= depend_on test-dep\n<%= File.read('#{dep}') %>")
+      write(dep, "a;")
+      asset      = asset('test-main.js')
+      old_digest = asset.hexdigest
+      old_uri    = asset.uri
+      assert_equal "a;\n", asset.to_s
+
+      write(dep, "b;")
+      asset = asset('test-main.js')
+      refute_equal old_digest, asset.hexdigest
+      refute_equal old_uri, asset.uri
+      assert_equal "b;\n", asset.to_s
+    end
+  end
+
+
   test "uri" do
     assert_equal "file://#{fixture_path_for_uri('asset/application.js')}?type=application/javascript&id=xxx",
       normalize_uri(@asset.uri)
@@ -1017,6 +1038,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",
@@ -1035,6 +1057,7 @@ def setup
 define("application.js", "application-955b2dddd0d1449b1c617124b83b46300edadec06d561104f7f6165241b31a94.js")
 define("application.css", "application-46d50149c56fc370805f53c29f79b89a52d4cc479eeebcdc8db84ab116d7ab1a.css")
 define("POW.png", "POW-1da2e59df75d33d8b74c3d71feede698f203f136512cbaab20c68a5bdebd5800.png")
+;
     EOS
 
     assert_equal [
@@ -1189,7 +1212,7 @@ 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/test_context.rb

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

test/test_engines.rb

@@ -30,7 +30,7 @@ class TestEngines < Sprockets::TestCase
     env = new_environment
     env.register_engine ".alert", AlertProcessor
     asset = env["hello.alert"]
-    assert_equal 'alert("Hello world!\n");', asset.to_s
+    assert_equal "alert(\"Hello world!\\n\");\n", asset.to_s
     assert_equal 'application/javascript', asset.content_type
   end
 

test/test_environment.rb

@@ -796,7 +796,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

@@ -100,16 +100,21 @@ def test_string_ends_with_semicolon
   end
 
   def test_concat_javascript_sources
-    assert_equal "var foo;\n", concat_javascript_sources("", "var foo;\n".freeze)
-    assert_equal "\nvar foo;\n", concat_javascript_sources("\n", "var foo;\n".freeze)
-    assert_equal " \nvar foo;\n", concat_javascript_sources(" ", "var foo;\n".freeze)
-
-    assert_equal "var foo;\nvar bar;\n", concat_javascript_sources("var foo;\n", "var bar;\n".freeze)
-    assert_equal "var foo;\nvar bar", concat_javascript_sources("var foo", "var bar".freeze)
-    assert_equal "var foo;\nvar bar;", concat_javascript_sources("var foo;", "var bar;".freeze)
-    assert_equal "var foo;\nvar bar;", concat_javascript_sources("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]