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

RPC wrappers, pass arguments by const ref #12504

Closed
gusev-p opened this issue Jan 12, 2023 · 5 comments · Fixed by #14003
Closed

RPC wrappers, pass arguments by const ref #12504

gusev-p opened this issue Jan 12, 2023 · 5 comments · Fixed by #14003
Assignees
@gusev-p @gleb-cloudius
Milestone
5.4

Comments

@gusev-p
Copy link

gusev-p commented Jan 12, 2023 •
edited

Now they are always passed by value, which sometimes leads to redundant copies, e.g. in storage_proxy::handle_write forward_fn from receive_mutation_handler declares the parameter as const frozen_mutation& m, but then it’s passed to this wrapper and gets copied, so we would have n copies for each forwarding.

The parameters are serialised synchronously so there should be no lifetime issues.

This was not the case before, but the behaviour changed in this commit. Nowdays there is no problem to get an object by reference during sending.
@gusev-p gusev-p self-assigned this Jan 12, 2023
@avikivity
Copy link
Member

Changed? That's the commit that introduced messaging_service.

@gusev-p
Copy link
Author

gusev-p commented Jan 12, 2023

Changed? That's the commit that introduced messaging_service.

I copied this from @gleb reply in the slack channel, didn't investigated it on my own yet.

@gleb-cloudius
Copy link
Contributor

gleb-cloudius commented Jan 12, 2023 •
edited

Changed? That's the commit that introduced messaging_service.

This scylladb/seastar@3942546 commit.
I probably checked what was first this change or the massaging service was introduced and pasted the wrong commit hash.

@DoronArazii DoronArazii added this to the 5.x milestone Jan 24, 2023
gusev-p pushed a commit to gusev-p/scylla that referenced this issue May 23, 2023
storage_proxy: mutation:: make frozen_mutation [[ref]]
fb2223f 
We had a redundant copy in receive_mutation_handler
forward_fn callback. This frozen_mutation is
dynamically allocated and can be arbitrary large.

Fixes: scylladb#12504
@gusev-p gusev-p mentioned this issue May 23, 2023
Merged
@gusev-p gusev-p assigned kbr-scylla and unassigned kbr-scylla May 23, 2023
avikivity added a commit that referenced this issue May 30, 2023
@avikivity
Merge 'RPC: add [[ref]] attribute to heavy parameters' from Gusev Petr
bb361f4 
By default `idl-compiler.py` emits code to pass parameters by value. There was an attribute `[[ref]]`, which makes it to use `const&`, but it was not used systematically and in many cases parameters were redundantly copied. In this PR, all `verb` directives have been reviewed and the `[[ref]]` attribute has been added where it makes sense.

The parameters [are serialised synchronously](https://github.com/scylladb/seastar/blob/master/include/seastar/rpc/rpc_impl.hh#L471) so there should be no lifetime issues. This was not the case before, but the behaviour changed in [this commit](scylladb/seastar@3942546). Now it's not a problem to get an object by reference when using `send_` methods.

Fixes: #12504

Closes #14003

* github.com:scylladb/scylladb:
  tracing::trace_info: pass by ref
  storage_proxy: pass inet_address_vector_replica_set by ref
  raft: add [[ref]] attribute
  repair: add [[ref]] attribute
  forward_request: add [[ref]] attribute
  storage_proxy: paxos:: add [[ref]] attribute
  storage_proxy: read_XXX:: make read_command [[ref]]
  storage_proxy: hint_mutation:: make frozen_mutation [[ref]]
  storage_proxy: mutation:: make frozen_mutation [[ref]]
@DoronArazii DoronArazii modified the milestones: 5.x, 5.4 Jun 1, 2023
margdoc pushed a commit to margdoc/scylla that referenced this issue Jun 6, 2023
@margdoc
storage_proxy: mutation:: make frozen_mutation [[ref]]
d4354e3 
We had a redundant copy in receive_mutation_handler
forward_fn callback. This frozen_mutation is
dynamically allocated and can be arbitrary large.

Fixes: scylladb#12504
denesb pushed a commit that referenced this issue Dec 18, 2023
@denesb
storage_proxy: mutation:: make frozen_mutation [[ref]]
62ae211 
We had a redundant copy in receive_mutation_handler
forward_fn callback. This frozen_mutation is
dynamically allocated and can be arbitrary large.

Fixes: #12504
(cherry picked from commit 5adbb6c)
@denesb
Copy link
Contributor

denesb commented Dec 18, 2023

Minor improvement, but the fix is very low-risk, so I'll backport.

@denesb
Copy link
Contributor

denesb commented Dec 18, 2023

Backported to 5.2.

denesb pushed a commit that referenced this issue Dec 18, 2023
@denesb
storage_proxy: mutation:: make frozen_mutation [[ref]]
27adf34 
We had a redundant copy in receive_mutation_handler
forward_fn callback. This frozen_mutation is
dynamically allocated and can be arbitrary large.

Fixes: #12504
(cherry picked from commit 5adbb6c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gusev-p gusev-p

@gleb-cloudius gleb-cloudius

Labels
None yet
Projects
None yet
Milestone
5.4
Development

Successfully merging a pull request may close this issue.

RPC: add [[ref]] attribute to heavy parameters gusev-p/scylla
7 participants
@avikivity @denesb @gusev-p @gleb-cloudius @scylladb-promoter @DoronArazii @kbr-scylla