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

Optimize span constructor #1274

Merged
merged 16 commits into from
Jan 13, 2022
Merged

Optimize span constructor #1274

merged 16 commits into from
Jan 13, 2022

Conversation

Jean85
Copy link
Collaborator

@Jean85 Jean85 commented Jan 11, 2022

This started as an investigation over #1273. I tried to run PHPBench over the Span constructor, and this is the result applying 5f94026:
immagine

Results seems promising, but the operation seems very small to obtain a consistent bench; @Ocramius any suggestions? You can also try this branch if you want to check if there's an improvement on your profiled test suite.

@Ocramius
Copy link

@Jean85 this is a good start, and we can iterate on it after the benchmarks are committed to mainline

@Jean85
Copy link
Collaborator Author

Jean85 commented Jan 11, 2022

@Ocramius do you agree that we shouldn't put this in CI? In that case, how would you generate a sensible baseline? We would have to re-run them every time manually?

@Ocramius
Copy link

@Ocramius do you agree that we shouldn't put this in CI?

TBH, having it in CI is generally good value: obviously trouble for older PHP versions, but it doesn't need to run on 7.2.

The reason for having it in CI is not to verify performance, but just to make sure that the benchmarks can still be run, even after years: otherwise, they quickly decay, and become unusable.

@Ocramius
Copy link

What you can do in CI is running with --revs=1 --iterations=1 - that makes sure that the bench is still usable.

@Jean85
Copy link
Collaborator Author

Jean85 commented Jan 11, 2022

@ste93cry this is ready for review.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

This seems like a good idea 👍 happy to see you guys diving so deep in the performance 💪

composer.json Outdated Show resolved Hide resolved
tests/Benchmark/SpanBench.php Show resolved Hide resolved
tests/Benchmark/SpanBench.php Show resolved Hide resolved
tests/Benchmark/SpanBench.php Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
src/Tracing/Span.php Outdated Show resolved Hide resolved
@ste93cry ste93cry merged commit 7ee1505 into master Jan 13, 2022
@ste93cry ste93cry deleted the profile-span-constructor branch January 13, 2022 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sentry\Tracing\Span::__construct() and Sentry\Tracing\Span#startChild() taking around 3% of execution time
4 participants