What is the point of avoiding the semicolon in concat_javascript_sources

[SamSaffron]

  def concat_javascript_sources(buf, source)
      if buf.bytesize > 0
        buf << ";" unless string_end_with_semicolon?(buf)
        buf << "\n" unless buf.end_with?("\n")
      end
      buf << source
    end

Has a check for

 buf << ";" unless string_end_with_semicolon?(buf)

I was debugging our painfully slow reload times in Discourse when running qunit.

Essentially after any edit of any js file if we reload /qunit it takes us about 10 seconds for the page to render.

Change concat javascript sources to

  def concat_javascript_sources(buf, source)
      if buf.bytesize > 0
        buf << ";"
        buf << "\n" unless buf.end_with?("\n")
      end
      buf << source
    end

And we shave off 6 or so seconds, that is huge.

So I wonder, why not simply do:

  def concat_javascript_sources(buf, source)
      if buf.bytesize > 0
        buf << ";"
        buf << "\n"
      end
      buf << source
    end

Sure, you have a few extra newlines and semicolons, but the minifier will remove them anyway so no harm. And no need to walk backwards through all these strings which is surprisingly inefficient in Ruby.

cc @schneems

[michaelherold]

I think this sounds like a great idea. I feel like the walk in string_end_with_semicolon? is unnecessarily expensive when having an extra semicolon doesn't invalidate any JavaScript syntax.

Since the common problem with concatenating JavaScript files is the lack of semicolons, automatically adding one (that, like Sam said, will then be removed by the minifier if it's unnecessary) seems on the surface to be a perfectly fine speed optimization.

@SamSaffron did you purposefully keep the two separate shift operators in your optimized example? I'm wondering why we wouldn't just collapse them down into one, a la:

def concat_javascript_sources(buf, source)
  if buf.bytesize > 0
    buf << ";\n"
  end
  buf << source
end

I benchmarked the difference in a little toy example that I'm posting below. Granted it's a toy example using StringIO, but reducing it down to one call significantly speeds up the operation.

require "benchmark/ips"

str = StringIO.new
str2 = StringIO.new

Benchmark.ips do |x|
  x.report("double") { str << ";"; str << "\n" }
  x.report("single") { str2 << ";\n" }

  x.compare!
end

Warming up --------------------------------------
              double   188.213k i/100ms
              single   254.231k i/100ms
Calculating -------------------------------------
              double      3.108M (± 1.1%) i/s -     15.622M in   5.026547s
              single      5.204M (± 3.0%) i/s -     26.186M in   5.036719s

Comparison:
              single:  5204430.5 i/s
              double:  3108200.0 i/s - 1.67x slower

[SamSaffron]

yeah, that was a bit lazy of me Michael, yeah a single shift operator would
be totally fine here.

On Sun, May 15, 2016 at 7:01 AM, Michael Herold [email protected]
wrote:

I think this sounds like a great idea. I feel like the walk in
string_end_with_semicolon? is unnecessarily expensive when having an
extra semicolon doesn't invalidate any JavaScript syntax.

Since the common problem with concatenating JavaScript files is the lack
of semicolons, automatically adding one (that, like Sam said, will then be
removed by the minifier if it's unnecessary) seems on the surface to be a
perfectly fine speed optimization.

@SamSaffron https://github.com/SamSaffron did you purposefully keep the
two separate shift operators in your optimized example? I'm wondering why
we wouldn't just collapse them down into one, a la:

def concat_javascript_sources(buf, source)
if buf.bytesize > 0
buf << ";\n"
end
buf << sourceend

I benchmarked the difference in a little toy example that I'm posting
below. Granted it's a toy example using StringIO, but reducing it down to
one call significantly speeds up the operation.

require "benchmark/ips"

str = StringIO.new
str2 = StringIO.new
Benchmark.ips do |x|
x.report("double") { str << ";"; str << "\n" }
x.report("single") { str2 << ";\n" }

x.compare!end

Warming up --------------------------------------
double 188.213k i/100ms
single 254.231k i/100ms
Calculating -------------------------------------
double 3.108M (± 1.1%) i/s - 15.622M in 5.026547s
single 5.204M (± 3.0%) i/s - 26.186M in 5.036719s

Comparison:
single: 5204430.5 i/s
double: 3108200.0 i/s - 1.67x slower

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#300 (comment)

[schneems]

This seems fine, send a patch?

[bouk]

@SamSaffron I was debugging the same thing and figured out that the issue is unicode (in case you were curious), here's a PR for that: #311