Use +@ instead of dup for duplicating strings

[nirebu]

String#+@ was introduced in Ruby 2.3: it is faster and a bit cheaper on
the memory side as well than #dup. However, to retain the helper contract
we need to return "thawed" copies of the original values, so we need to
first call String#-@ to return a frozen copy, then call String#+@ to
effectively have an unfrozen copy of the original value.

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
  gem "benchmark-memory"
end

Benchmark.ips do |x|
  x.report("+(-@)") { v = "foo"; +(-v) }
  x.report("dup") { v = "foo"; v.dup }

  x.compare!
end

Benchmark.memory do |x|
  x.report("+(-@)") { v = "foo"; +(-v) }
  x.report("dup") { v = "foo"; v.dup }

  x.compare!
end

Warming up --------------------------------------
               +(-@)   781.717k i/100ms
                 dup   711.791k i/100ms
Calculating -------------------------------------
               +(-@)      7.801M (± 0.8%) i/s -     39.086M in   5.010682s
                 dup      7.138M (± 0.7%) i/s -     36.301M in   5.085850s

Comparison:
               +(-@):  7800971.1 i/s
                 dup:  7138065.1 i/s - 1.09x  slower

Calculating -------------------------------------
               +(-@)    80.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)
                 dup    80.000  memsize (     0.000  retained)
                         2.000  objects (     0.000  retained)
                         1.000  strings (     0.000  retained)

Comparison:
               +(-@):         80 allocated
                 dup:         80 allocated - same

What does this PR do?
Changes how the gem duplicates frozen strings, in favor of a more performant method.

Motivation
I was benchmarking a client application and found the modified line as a source of retained strings. Even if I couldn't replicate the issue in the synthetic benchmark, this change reduced the number of retained strings coming from that allocation.

Additional Notes
Additional ref: rails/rails@1b86d90

How to test the change?

Please find above the code for benchmarking the change.

[TonyCTHsu]

👋 @nirebu , I am aware that In Ruby 2.3 or later unary plus operator is faster than String#dup is documented. However, this would break our support policy for Ruby 2.1/2.2.

Our team is currently planning the deprecation for those old rubies, perhaps we would introduce the changes by then

[nirebu]

@TonyCTHsu thanks for getting back. Totally agree with you, judging by the broken tests I can guess this is not the right time for this.

Closing this then :) Will get back to it in the future!

[marcotc]

@nirebu, given this is a hot code path, could you add a guard around old ruby versions and actually implement this for newer rubies? Something like:

# Introduced in Ruby 2.3
if String.method_defined?(:+@)
  def self.frozen_or_dup(v)
    v.frozen? ? v : +v
  end
else
  def self.frozen_or_dup(v)
    v.frozen? ? v : v.dup
  end
end

[nirebu]

@nirebu, given this is a hot code path, could you add a guard around old ruby versions and actually implement this for newer rubies?

@marcotc It's a hot code path indeed, and thanks for the suggestion. I can take care of it.

I will take a look at the failing specs as well.

[nirebu]

@marcotc I've updated my PR with your suggestion, however the failing specs tell a different story: it seems that it is mandatory to have a different object_id on the frozen_or_dup return value.

Given how String#+@ returns self if the string is not frozen, and if this behaviour is desirable, the specs should be tweaked as well to just check the value instead of be_a_frozen_copy_of, which in turns expects different object_ids.

I have yet to dig in how these Strings are used downstream, but it seems that we want different objects given this comment.

EDIT: I've read #1783 's discussion, and it seems that changing the behaviour to use +@ is a breaking change. As far as I understood, users are able to modify spans, and having non-copies of those objects could lead to some undesirable side effects. I say this because we would eventually call freeze on something that the library doesn't expect to become frozen if it wasn't in the first place.

[marcotc]

Hey, @nirebu you are completely right, and sorry for us not catching this earlier.

We could technically use -@ to return a frozen copy of the String or itself if already frozen: it would provide us the same level of confidence that the String value won't change on us during processing.
This is a semantic change, but if all tests pass it might be promising.

One issue might be related to users of the the Processing Pipeline, given you are allowed to arbitrarily modify the span: it's possible users are performing modifying operation on Strings belonging to a trace or span.

[nirebu]

@marcotc I've updated my PR (and main comment). The full explanation is in the commit message and main comment, however I think I've found a way to squeeze a bit more performance while keeping the same contract for the helper: if v is frozen, return it, otherwise return a non-frozen copy (which may end up or not end up frozen where it's used).

Another optimisation I can think of would be to have a specialised helper to use in theDatadog::Tracing::Correlation::Identifier class, where the results are then frozen again. I've added a commit to add this, let me know what you think.

[marcotc]

@nirebu, can you leave a comment here explaining why +(-v) is being done here? I think this will raise questions in the future when someone stumbles upon this line.

[nirebu]

@marcotc added!

[marcotc]

@nirebu, the Datadog::Tracing::Correlation::Identifier changes look good, thank you!

[nirebu]

@marcotc I should have addressed all feedback.

[nirebu]

@marcotc is there anything else I can do here?

[marcotc]

Thank you so much, @nirebu! 🙇