optimize XML serialization

[ThomasSevestre]

This PR improves performance of XML serialization :

• by 15% without active support
• by 25% when active support is loaded

This has a visible impact in our application using ActiveRecord. I have a 15% overall gain.

I'm using this script to benchmark :

# require "active_support/all"
require "benchmark"
module Xlsxtream
  n = 500000
  Benchmark.bm do |x|
    x.report { for i in 1..n; Row.new(['foo', nil, 23, 'foo', nil, 23, 'foo', nil, 23, 'foo', nil, 23], 1).to_xml; end }
  end
end

[julik]

@ThomasSevestre this looks amazing but I believe extending core classes in a "leaf" library is not a great idea. Can we achieve the same with a module in the gem that would contain all those helper methods, without core namespace alterations?

[felixbuenemann]

I’m against patching core classes, too. I’m also curious as to why this is faster than the existing code. Would you care to explain?

[ThomasSevestre]

I do not like patching core classes as well... I'm not sure it is possible to achieve the same performance in a different way.

In current code, the expensive part is the big case when :

case value
when Numeric
when TrueClass, FalseClass
when Time
when DateTime
when Date
else
end

It is using === operator multiple times on value.
This operator is overriden by active support, which explains why it has more impact when the gem is loaded.
Switching to a simple method call is faster because method calls are highly optimized by ruby core team.

This trick is also used by rails teams, here for instance : rails/rails@d36eb239

[julik]

The Rails team is at liberty adding methods to core classes, but this is not the liberty authors of "leaf" libraries have - just due to the adoption.

[ThomasSevestre]

Yes. I'm not trying to advocate for core class patches, just explaining the performance gain.

It may be acceptable to load this optimisation explicitly ?

Like this for instance :

require "xlsxstream"
require "xlsxstream/core_extension"

[ThomasSevestre]

Another solution would be to optimize for String and nil which are probably the most common types.

It would look like this :

case value
when nil
  # noop
when String
when Numeric
when TrueClass, FalseClass
when Time
when DateTime
when Date
else
  # recursive call to handle value.to_s
end

It will ensure good performance for common types.

What do you think ?

[felixbuenemann]

@ThomasSevestre I like the idea with the optimized case statement. Could you benchmark that version?
Also is “when nil” faster then “when NilClass”, which would look nicer.