TimeWithZone#xmlschema should be able to display more than 6 digits

[mtsmfm]

No description provided.

[arthurnn]

I guess this seems good.. @pixeltrix thoughts?

[pixeltrix]

@mtsmfm what about performance when generating large amounts of JSON responses? Seems like interpolating a string every time would be slower - can you create a quick benchmark?

[pixeltrix]

@mtsmfm I think we should do this, I just think the implementation might be slow - one option would be to pre-define all of the format strings, e.g:

PRECISION = [
  0 => '%FT%T'.freeze,
  1 => '%FT%T.%1N'.freeze,
  # more definitions
  9 => '%FT%T.%9N'.freeze
]

def xmlschema(fraction_digits = 0)
  "#{time.strftime(PRECISION[fraction_digits.to_i])}#{formatted_offset(true, 'Z')}"
end

Can you do a comparison with this as well - thanks?

[mtsmfm]

I compared implementations.
This patch is faster than original.
Your suggestion (cache format string) is faster than my patch.
I'm not sure that we should accept this complexity or not.

The following is result:

require 'benchmark/ips'
require 'active_support'
require 'active_support/time_with_zone'

module ActiveSupport
  class TimeWithZone
    def original(fraction_digits = 0)
      fraction = if fraction_digits.to_i > 0
        (".%06i" % time.usec)[0, fraction_digits.to_i + 1]
      end

      "#{time.strftime("%Y-%m-%dT%H:%M:%S")}#{fraction}#{formatted_offset(true, 'Z')}"
    end

    def patched1(fraction_digits = 0)
      fraction_digits = fraction_digits.to_i
      format = "%FT%T"
      format << ".%#{fraction_digits}N" if fraction_digits > 0

      "#{time.strftime(format)}#{formatted_offset(true, 'Z')}"
    end

    PRECISIONS = Hash.new {|hash, fraction_digits|
      hash[fraction_digits] =
        if fraction_digits == 0
          "%FT%T"
        else
          "%FT%T.%#{fraction_digits}N"
        end
    }

    def patched2(fraction_digits = 0)
      format = PRECISIONS[fraction_digits.to_i]

      "#{time.strftime(format)}#{formatted_offset(true, 'Z')}"
    end
  end
end

Benchmark.ips do |x|
  time = ActiveSupport::TimeWithZone.new(Time.utc(2000, 1, 1, 0, 0, 0), ActiveSupport::TimeZone['UTC'])
  x.report("original") { (0..10).each {|i| time.original(i) } }
  x.report("patched1") { (0..10).each {|i| time.patched1(i) } }
  x.report("patched2") { (0..10).each {|i| time.patched2(i) } }
  x.compare!
end

__END__
$ ruby -v benchmark.rb
ruby 2.2.3p173 (2015-08-18 revision 51636) [x86_64-linux]
Calculating -------------------------------------
            original     2.250k i/100ms
            patched1     2.970k i/100ms
            patched2     3.311k i/100ms
-------------------------------------------------
            original     23.417k (± 4.6%) i/s -    117.000k
            patched1     30.387k (± 6.7%) i/s -    151.470k
            patched2     34.935k (± 5.6%) i/s -    175.483k

Comparison:
            patched2:    34934.7 i/s
            patched1:    30387.4 i/s - 1.15x slower
            original:    23416.9 i/s - 1.49x slower

[pixeltrix]

If we preset the 0 index on the PRECISIONS constant then the implementation isn't that complex plus it has the added benefit of creating less string objects.

Here's the modified benchmark:

require 'benchmark/ips'
require 'active_support/time'

module ActiveSupport
  class TimeWithZone
    PRECISIONS = Hash.new { |h, n| h[n] = "%FT%T%#{n}N".freeze }
    PRECISIONS[0] = '%FT%T'.freeze

    def xmlschema2(fraction_digits = 0)
      fraction_digits = fraction_digits.to_i
      format = "%FT%T"
      format << ".%#{fraction_digits}N" if fraction_digits > 0

      "#{time.strftime(format)}#{formatted_offset(true, 'Z')}"
    end

    def xmlschema3(fraction_digits = 0)
      "#{time.strftime(PRECISIONS[fraction_digits.to_i])}#{formatted_offset(true, 'Z'.freeze)}"
    end
  end
end

Time.zone = 'UTC'
time = Time.current
precision = ARGV.first.to_i

def count_strings(method)
  strings_at_start = ObjectSpace.count_objects[:T_STRING]
  yield
  strings_at_end = ObjectSpace.count_objects[:T_STRING]
  puts "#{method} allocated #{strings_at_end - strings_at_start} strings"
end

Benchmark.ips do |x|
  x.report("xmlschema(#{precision})")  { time.xmlschema(precision)  }
  x.report("xmlschema2(#{precision})") { time.xmlschema2(precision) }
  x.report("xmlschema3(#{precision})") { time.xmlschema3(precision) }

  x.compare!
end

GC.disable

count_strings("xmlschema(#{precision})")  { time.xmlschema(precision)  }
count_strings("xmlschema2(#{precision})") { time.xmlschema2(precision) }
count_strings("xmlschema3(#{precision})") { time.xmlschema3(precision) }

If you run it with 0 for the precision then caching the format only gives you a small improvement:

$ ruby -v benchmark.rb 0
ruby 2.2.3p173 (2015-08-18 revision 51636) [x86_64-darwin14]
Calculating -------------------------------------
        xmlschema(0)    23.378k i/100ms
       xmlschema2(0)    24.137k i/100ms
       xmlschema3(0)    24.827k i/100ms
-------------------------------------------------
        xmlschema(0)    509.633k (± 1.1%) i/s -      2.548M
       xmlschema2(0)    551.799k (± 1.1%) i/s -      2.776M
       xmlschema3(0)    582.415k (± 1.0%) i/s -      2.930M

Comparison:
       xmlschema3(0):   582415.0 i/s
       xmlschema2(0):   551799.1 i/s - 1.06x slower
        xmlschema(0):   509632.8 i/s - 1.14x slower

xmlschema(0) allocated 34 strings
xmlschema2(0) allocated 7 strings
xmlschema3(0) allocated 4 strings

However, once you add in the fractional digits then the performance improvement and memory saving is more marked:

$ ruby -v benchmark.rb 6
ruby 2.2.3p173 (2015-08-18 revision 51636) [x86_64-darwin14]
Calculating -------------------------------------
        xmlschema(6)    16.574k i/100ms
       xmlschema2(6)    18.004k i/100ms
       xmlschema3(6)    20.269k i/100ms
-------------------------------------------------
        xmlschema(6)    280.658k (± 2.7%) i/s -      1.409M
       xmlschema2(6)    314.009k (± 2.1%) i/s -      1.584M
       xmlschema3(6)    379.726k (± 1.9%) i/s -      1.905M

Comparison:
       xmlschema3(6):   379725.8 i/s
       xmlschema2(6):   314008.6 i/s - 1.21x slower
        xmlschema(6):   280657.7 i/s - 1.35x slower

xmlschema(6) allocated 37 strings
xmlschema2(6) allocated 10 strings
xmlschema3(6) allocated 5 strings

If your API endpoint is generating a lot of timestamp strings then I think this is definitely worth any perceived complexity - can you adjust your PR, thanks.

[mtsmfm]

Thank you for your reviewing ⭐
Changed and force pushed 😄

[pixeltrix]

Forcing a rebuild on Travis