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

fix: Small perf improvement to generate_r #1522

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

fbogsany
Copy link
Contributor

This is a small performance improvement to the generate_r method for the consistent-probability sampler. This method generates a r value from the trace_id. The current implementation unnecessarily allocates a slice of the trace_id string. Using the offset: argument to unpack1, we can avoid this allocation.

Benchmark results:

Calculating -------------------------------------
          generate_r      3.920M (± 0.4%) i/s -     19.654M
 generate_r_in_place      4.596M (± 0.5%) i/s -     23.085M

Comparison:
 generate_r_in_place:  4595613.7 i/s
          generate_r:  3920215.1 i/s - 1.17x slower

@fbogsany fbogsany merged commit 37e225a into main Sep 14, 2023
48 checks passed
@robertlaurin robertlaurin deleted the optimize-generate_r branch September 14, 2023 19:14
@@ -106,7 +106,7 @@ def decimal(str)
end

def generate_r(trace_id)
x = trace_id[8, 8].unpack1('Q>') | 0x3
x = trace_id.unpack1('Q>', offset: 8) | 0x3
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you are aware but offset was added in 3.1, and IIRC otel is supposed to be compatible with Ruby 3.0?

But since the offset is static here, you can use the @ syntax:

>> "abcedfghsdfdsfhdsfjsdkjfdsjfkdlsfjdsfjksdfk".unpack1('Q>', offset: 8)
=> 8314883393651632228
>> "abcedfghsdfdsfhdsfjsdkjfdsjfkdlsfjdsfjksdfk".unpack1('@8Q>')
=> 8314883393651632228

offset was added to avoid allocating a new pattern string every time when the offset is dynamic.

@@ -106,7 +106,7 @@ def decimal(str)
end

def generate_r(trace_id)
x = trace_id[8, 8].unpack1('Q>') | 0x3
x = trace_id.unpack1('Q>', offset: 8) | 0x3
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you are aware but offset was added in 3.1, and IIRC otel is supposed to be compatible with Ruby 3.0?

But since the offset is static here, you can use the @ syntax:

>> "abcedfghsdfdsfhdsfjsdkjfdsjfkdlsfjdsfjksdfk".unpack1('Q>', offset: 8)
=> 8314883393651632228
>> "abcedfghsdfdsfhdsfjsdkjfdsjfkdlsfjdsfjksdfk".unpack1('@8Q>')
=> 8314883393651632228

offset was added to avoid allocating a new pattern string every time when the offset is dynamic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants