Add TimedPut to stress test #12559
Add TimedPut to stress test #12559
Conversation
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jowlyzhang
force-pushed
the
stress_timed_put
branch
from
4464a70
to
5febb02
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
jowlyzhang
force-pushed
the
stress_timed_put
branch
from
5febb02
to
6931466
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jowlyzhang
force-pushed
the
stress_timed_put
branch
from
6931466
to
507bfa5
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -1304,6 +1304,14 @@ class NonBatchedOpsStressTest : public StressTest { | |||
| (value_base % FLAGS_use_put_entity_one_in) == 0) { | |||
| s = db_->PutEntity(write_opts, cfh, k, | |||
| GenerateWideColumns(value_base, v)); | |||
| } else if (FLAGS_use_timed_put_one_in > 0 && | |||
| (value_base % FLAGS_use_timed_put_one_in) == 0) { | |||
There was a problem hiding this comment.
If use_put_entity_one_in and use_timed_put_one_in have common factors (or are equal) then then the actual frequency of TimedPut is really messed up or zero. A partial fix would add a reasonably large prime constant to value_base before this mod operation.
cache_bench_tool.cc (https://github.com/facebook/rocksdb/blob/main/cache/cache_bench_tool.cc) has an example of efficiently, randomly choosing disjoint actions with chosen frequencies. Search for lookup_insert_threshold. (Modulo operator is slow btw.)
There was a problem hiding this comment.
That's a great point! I was only thinking that used_timed_put_one_in will be inaccurate if put entity is also enabled, but didn't realize in the case when they have a common factor, the former will be as if it's unset. Thanks for calling that out and the example for how to solve it. Let me add the partial fix of adding a reasonable large prime. And I will convert these one_in type of arguments to be percentage based and use the strategy in cache bench tool for them in a follow up.
There was a problem hiding this comment.
one_in is the standard for db_stress, so that's fine. I was just pointing to the cache_bench code for an example of randomized behavior with exclusive probabilities.
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
db/write_batch.cc
Outdated
| PutFixed64(&encoded_write_time, write_unix_time); | ||
| std::array<Slice, 2> value_with_time{{value, encoded_write_time}}; | ||
| SliceParts packed_value(value_with_time.data(), 2); | ||
| PutLengthPrefixedSliceParts(&b->rep_, packed_value); |
There was a problem hiding this comment.
I guess I was hoping to get rid of the SliceParts stuff. Here we're fundamentally putting the packed value contiguously in &b->rep_ so it seems like we can use that for protection info rather than doing a bunch of extra copies for SliceParts stuff.
There was a problem hiding this comment.
I agree, I started with this hoping that it could save copying to a string buffer, but the SliceParts used by the protection info will create such a string buffer anyway, so this is unnecessarily hacky.
| PutFixed64(&encoded_write_time, unix_write_time); | ||
| std::array<Slice, 2> value_with_time{{val, encoded_write_time}}; | ||
| SliceParts packed_value(value_with_time.data(), 2); | ||
| return UpdateProtInfo(cf, key, packed_value, kTypeValuePreferredSeqno); |
There was a problem hiding this comment.
Here the data comes unpacked, and we have to physically or virtually pack it. SliceParts makes sense here, especially if we end up fixing this TODO: https://github.com/facebook/rocksdb/blob/9.1.fb/util/hash.cc#L91
jowlyzhang
force-pushed
the
stress_timed_put
branch
from
ea5f87d
to
d92afc1
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in 8b3d9e6. |
This also updates WriteBatch's protection info to include write time since there are several places in memtable that by default protects the whole value slice.