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

partition_snapshot_reader: do not accidentally copy schema #9507

Closed

Conversation

enedil
Copy link
Contributor

@enedil enedil commented Oct 20, 2021

Functions upper_bound and lower_bound had signatures:

template<typename T, typename... Args>
static rows_iter_type lower_bound(const T& t, Args... args);

This caused a dacay from const schema& to schema as one of the args,
which in turn copied the schema in a fair number of the queries. Fix
that by setting the parameter type to Args&&, which doesn't discard
the reference.

Fixes #9502

Functions `upper_bound` and `lower_bound` had signatures:
```
template<typename T, typename... Args>
static rows_iter_type lower_bound(const T& t, Args... args);
```
This caused a dacay from `const schema&` to `schema` as one of the args,
which in turn copied the schema in a fair number of the queries. Fix
that by setting the parameter type to `Args&&`, which doesn't discard
the reference.

Fixes scylladb#9502
Copy link
Contributor

@haaawk haaawk left a comment

Choose a reason for hiding this comment

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

LGTM

benipeled pushed a commit to benipeled/scylla that referenced this pull request May 31, 2022
Functions `upper_bound` and `lower_bound` had signatures:
```
template<typename T, typename... Args>
static rows_iter_type lower_bound(const T& t, Args... args);
```
This caused a dacay from `const schema&` to `schema` as one of the args,
which in turn copied the schema in a fair number of the queries. Fix
that by setting the parameter type to `Args&&`, which doesn't discard
the reference.

Fixes scylladb#9502

Closes scylladb#9507

(cherry picked from commit 9caf85f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

50% system perf regression in memtable reader
2 participants