Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce generated Span ID range to fit in Fixnum #1189

Merged
merged 1 commit into from Sep 29, 2020
Merged

Conversation

marcotc
Copy link
Member

@marcotc marcotc commented Sep 28, 2020

Ruby transparently handles small and large integers using the Integer object. Behind the scenes, a number could either be a Fixnum or Bignum.

A Fixnum "Holds Integer values that can be represented in a native machine word (minus 1 bit)", and thus requiring no auxiliary memory to be represented.
Fixnums are faster to create and manipulate.

The transparent boundary between Fixnums and Bignums, in the positive integer range, is around 2**62:

require 'objspace'
ObjectSpace.memsize_of(2**62-1)
# => 0
ObjectSpace.memsize_of(2**62)
# => 40

In the tracer, we were generating random span ids up to 2**63, meaning half the generated ids were internally Bignums.

This PR reduces the range of internally generated span ids** to up to 2**62-1. The limit of this new range is still sufficient to produce unique ids for the purpose of tracing.

This does not affect spans received from external sources: those are still handled as-is, without modification.

Also, this PR fixes our sampling logic that performs Knuth's sampling algorithm to use the correct maximum range: before we were using 2**63, but our specs require our sampling to be consistent across languages, specially when possibly handling externally provided span ids. The specs require 2**64 to be the sampling range limit. This fix can be seen in "Datadog::RateSampler#sample?".

Benchmark results

For our critical path benchmark, that measures span creation up until the hand-off to the background worker:

  • 1-6% wall time reduction
  • 4-6% memory reduction
  • 11-22% fewer objects created
Before [operations/sec]
     1 spans:   41298.16
    10 spans:    9982.20
   100 spans:    1239.96

After [operations/sec]
     1 spans:   41898.30
    10 spans:   10466.44
   100 spans:    1322.01

Comparison (% faster; slower if negative)
     1 spans:       1.45
    10 spans:       4.85
   100 spans:       6.62
Before [bytes allocated (objects created)]
     1 traces:   251760.00 (   2784.00)
    10 traces:  1825600.00 (  14464.00)
   100 traces: 17502136.00 ( 131539.00)

After  [bytes allocated (objects created)]
     1 traces:   239400.00 (   2475.00)
    10 traces:  1705920.00 (  11472.00)
   100 traces: 16299616.00 ( 101476.00)

Comparison (% reduction, increase negative)
     1 traces:       4.91 (     11.10)
    10 traces:       6.56 (     20.69)
   100 traces:       6.87 (     22.85)

@marcotc marcotc added core Involves Datadog core libraries performance Involves performance (e.g. CPU, memory, etc) labels Sep 28, 2020
@marcotc marcotc requested a review from a team September 28, 2020 22:00
@marcotc marcotc self-assigned this Sep 28, 2020
@brettlangdon
Copy link
Member

11-22% fewer objects created

What perceived impact does this have on an application?

@richardstartin
Copy link
Member

Whilst I can't dispute the local benefits, I'm not sure this a good idea because it makes it twice as likely that there will be a span ID collision with other applications in the trace, which might be node, Java, .NET, etc.

Copy link
Contributor

@ericmustin ericmustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the implementation itself lgtm so happy to approve, but i will defer to other folks on whether we want to introduce this change due to potential conflicts with other languages as mentioned by other reviewers.

Copy link

@raphaelgavache raphaelgavache left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The increased probability of spanID collision in a single trace is acceptable, lgtm

@marcotc marcotc merged commit 89dbf48 into master Sep 29, 2020
@marcotc marcotc deleted the perf/smaller-span-id branch September 29, 2020 20:08
@marcotc marcotc added this to the 0.41.0 milestone Sep 30, 2020
michaelkl pushed a commit to michaelkl/dd-trace-rb that referenced this pull request Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries performance Involves performance (e.g. CPU, memory, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants