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
internal/datastreams: Improve performance #2455
Conversation
9ddc60c
to
4221d83
Compare
BenchmarksBenchmark execution time: 2023-12-20 16:14:17 Comparing candidate commit f4ec4d8 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 40 metrics, 0 unstable metrics. scenario:BenchmarkSingleSpanRetention/no-rules-24
|
4221d83
to
206910f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs the big fix from the other repo but Lgtm otherwise.
var s strings.Builder | ||
l := 0 | ||
for _, t := range edgeTags { | ||
l += len(t) | ||
} | ||
l += 8 | ||
s.Grow(l) | ||
for _, t := range edgeTags { | ||
s.WriteString(t) | ||
} | ||
s.WriteByte(byte(parentHash)) | ||
s.WriteByte(byte(parentHash >> 8)) | ||
s.WriteByte(byte(parentHash >> 16)) | ||
s.WriteByte(byte(parentHash >> 24)) | ||
s.WriteByte(byte(parentHash >> 32)) | ||
s.WriteByte(byte(parentHash >> 40)) | ||
s.WriteByte(byte(parentHash >> 48)) | ||
s.WriteByte(byte(parentHash >> 56)) | ||
return s.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a small optimization but we could return early here if there are no edge tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 If there are no edgeTags, going through the two empty loops will be really quick (also, no edge tags should never happen if there are no bugs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good just a few small questions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! Thanks!
This adds a test for an issue that seems to have been introduced in: #2455
Since somebody mentioned it to me on slack, let me clarify something here: My commit 803b0d4 that references this PR was a deadend. I briefly took a look at this code because it seemed suspicious, but it held up to my scrutiny and I believe it's working correctly 👏. |
What does this PR do?
Before:
Now:
Motivation
For high throughput apps (tens of thousands of messages per second on a single host), the overhead of the tracer was pretty high (6% on a high throughput test application).
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!