storage_proxy: place unique_response_handler:s in small_vector instead of std::vector #8606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The previous vector->small_vector transition patch was based on typedefs and I think it was a good idea. It would be nicer to reduce the number of places where one has to type "3" to a single definition in storage_proxy.hh. |
service/storage_proxy.cc
Outdated
| @@ -2012,7 +2020,7 @@ storage_proxy::create_write_response_handler(const std::tuple<lw_shared_ptr<paxo | |||
| inet_address_vector_topology_change(), inet_address_vector_topology_change(), std::move(tr_state), get_stats(), std::move(permit)); | |||
| } | |||
|
|
|||
| void storage_proxy::register_cdc_operation_result_tracker(const std::vector<storage_proxy::unique_response_handler>& ids, lw_shared_ptr<cdc::operation_result_tracker> tracker) { | |||
| void storage_proxy::register_cdc_operation_result_tracker(const utils::small_vector<storage_proxy::unique_response_handler, 3>& ids, lw_shared_ptr<cdc::operation_result_tracker> tracker) { | |||
There was a problem hiding this comment.
Why do you optimize for a batch with up to three different statements? One should be a common case here.
There was a problem hiding this comment.
I thought we have one per replica. Agree it should be 1 if it's per-mutation.
I'll add a type name. |
We'd like to change the vector to a more involved type, so to avoid repeating it everywhere, give it a name. The actual type isn't changed in this patch.
small_vector wants the move constructor to be noexcept, and move assighment to exist (and be noexcept). These are easy to achieve.
…d of std::vector This cuts an allocation in the write path. Instruction count reduction isn't large, but performance does improve (results are consistent): before: 196369.48 tps ( 55.2 allocs/op, 13.2 tasks/op, 51658 insns/op) after: 199290.32 tps ( 54.2 allocs/op, 13.2 tasks/op, 51600 insns/op) (this is perf_simple_query --write --smp 1 --operations-per-shard 1000000)
avikivity
force-pushed
the
storage_proxy-response-id-small_vector
branch
from
484791a
to
dd904f7
Compare
|
Update:
|
nyh
approved these changes
May 9, 2021
nyh
added a commit
that referenced
this pull request
May 10, 2021
… instead of std::vector' from Avi Kivity This cuts an allocation in the write path. Instruction count reduction isn't large, but performance does improve (results are consistent): before: 196369.48 tps ( 55.2 allocs/op, 13.2 tasks/op, 51658 insns/op) after: 199290.32 tps ( 54.2 allocs/op, 13.2 tasks/op, 51600 insns/op) (this is perf_simple_query --write --smp 1 --operations-per-shard 1000000) Since small_vector requires noexcept move constructor and assignment, they corresponding unique_response_handler members are adjusted/added respectively. Closes #8606 * github.com:scylladb/scylla: storage_proxy: place unique_response_handler:s in small_vector instead of std::vector storage_proxy: make unique_response_handler friendly to small_vector storage_proxy: give a name to a vector of unique_response_handlers
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This cuts an allocation in the write path. Instruction count reduction isn't
large, but performance does improve (results are consistent):
before: 196369.48 tps ( 55.2 allocs/op, 13.2 tasks/op, 51658 insns/op)
after: 199290.32 tps ( 54.2 allocs/op, 13.2 tasks/op, 51600 insns/op)
(this is perf_simple_query --write --smp 1 --operations-per-shard 1000000)
Since small_vector requires noexcept move constructor and assignment,
they corresponding unique_response_handler members are adjusted/added
respectively.