-
Notifications
You must be signed in to change notification settings - Fork 1.1k
chore: prefetch keys in opmget #4778
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
base: main
Are you sure you want to change the base?
Conversation
2ae1ba8
to
e01e062
Compare
6336ef0
to
4084072
Compare
constexpr unsigned kPrefetchLimit = 32; | ||
if (mget_prefetch_keys) { | ||
unsigned prefetched = 0; | ||
for (string_view key : keys) { | ||
pt.Prefetch(key); | ||
if (++prefetched >= kPrefetchLimit) { | ||
break; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move it before key_index.reserve() or even higher to have more time for memory prefetching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Factor in the cost of computing key hashes twice 🙄
Hello 👨🍳 |
6ce9934
to
3704a3f
Compare
Also, stop deduplicating keys by default, but keep it as a run-time flag. All in all, this PR improves MGET (100% miss) throughput by at least 7%: from 1.23M to 1.32M qps. (100% miss traffic allows measuring more effectively the impact of this code) Also, finally fix TieredStorageTest.FlushAll test. Signed-off-by: Roman Gershman <roman@dragonflydb.io>
Also, stop deduplicating keys by default, but keep it as a run-time flag.
All in all, this PR improves MGET (100% miss) throughput by at least 7%:
from 1.23M to 1.32M qps.
(100% miss traffic allows measuring more effectively the impact of this code)
Also, finally fix TieredStorageTest.FlushAll test.