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.

Sign up for GitHub

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

Jump to bottom

Optimization: clientStreamInit() copied ClientStreamData #1801

Closed
wants to merge 4 commits into from
Closed

Optimization: clientStreamInit() copied ClientStreamData #1801

wants to merge 4 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented Apr 26, 2024 •
edited by rousskov
Loading

Detected by Coverity. CID 1529543 and CID 1529594: Unnecessary object
copies can affect performance (COPY_INSTEAD_OF_MOVE).

rousskov
rousskov requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Please apply applicable #1797 concerns to this PR. Please do not post new PRs adding std::move wrappers until the affected PRs land.

@rousskov rousskov changed the title perf nit: use std::move in parseHttpRequest Optimization: Use std::move in parseHttpRequest Apr 26, 2024
@kinkie kinkie changed the title Optimization: Use std::move in parseHttpRequest Optimization: avoid a copy clientStreamInsertHead Apr 30, 2024
@kinkie kinkie changed the title Optimization: avoid a copy clientStreamInsertHead Optimization: avoid a copy clientStreamInit Apr 30, 2024
@kinkie kinkie requested a review from rousskov April 30, 2024 10:06
yadij
yadij requested changes Apr 30, 2024
View reviewed changes
src/client_side.cc Outdated Show resolved Hide resolved
src/client_side.cc Outdated Show resolved Hide resolved
@yadij yadij added the S-waiting-for-author author action is expected (and usually required) label Apr 30, 2024
@rousskov rousskov changed the title Optimization: avoid a copy clientStreamInit Optimization: clientStreamInit() copied ClientStreamData Apr 30, 2024
rousskov
rousskov previously approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Thank you for optimizing this code. I am approving this PR after adjusting PR title/description and addressing Amos concerns, but I think we should do one more (similar) change in this PR.

src/client_side.cc Outdated Show resolved Hide resolved
src/client_side.cc Outdated Show resolved Hide resolved
src/clientStream.cc Outdated Show resolved Hide resolved
@kinkie kinkie requested review from yadij and rousskov May 1, 2024 09:49
@kinkie kinkie added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels May 1, 2024
@kinkie kinkie added the M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels label May 1, 2024
@kinkie
Copy link
Contributor Author

kinkie commented May 1, 2024

@yadij your concerns should have been addressed. Mind re-reviewing here? Thanks!

@yadij yadij removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 2, 2024
squid-anubis pushed a commit that referenced this pull request May 2, 2024
@kinkie @squid-anubis
Optimization: clientStreamInit() copied ClientStreamData (#1801)
2c3d6a4 
Detected by Coverity. CID 1529543 and CID 1529594: Unnecessary object
copies can affect performance (COPY_INSTEAD_OF_MOVE).
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label May 2, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels May 3, 2024
kinkie added a commit to kinkie/squid that referenced this pull request Jun 22, 2024
@kinkie
Optimization: clientStreamInit() copied ClientStreamData (squid-cache…
a9a75f9 
…#1801)

Detected by Coverity. CID 1529543 and CID 1529594: Unnecessary object
copies can affect performance (COPY_INSTEAD_OF_MOVE).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousskov rousskov rousskov approved these changes

@yadij yadij yadij approved these changes

Assignees
No one assigned
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kinkie @rousskov @yadij @squid-anubis