chore: Limit expiry value to 32 bits. #5797
Conversation
romange
force-pushed
the
Pr1
branch
4 times, most recently
from
8e2daae to
15a484c
Compare
|
the next step would be to start using |
There was a problem hiding this comment.
the next step would be to start using expire_base_ and generations to represent the absolute times using ~49 days ExpirePeriod. The absolute time := expire_base_ + expire_period so we should refresh expire_base_ daily to maintain 48-49 days span.
Not sure what the technical benefit of this would be. Not sure anyone would notice after 49 days 🤷🏻♂️ Who knows, it's not a long interval, especially for someone who has time sensitive data
Please see my comment around struct sizes
| VLOG(1) << "Performing flush"; | ||
| error_code ec = Drakarys(flush_trans.get(), DbSlice::kDbAll); | ||
| if (ec) { | ||
| LOG(ERROR) << "Error flushing db " << ec.message(); | ||
| } | ||
| Drakarys(flush_trans.get(), DbSlice::kDbAll); | ||
| } |
There was a problem hiding this comment.
I noticed this as well in #5821
| precision_ = 1; // seconds | ||
| if (ms < UINT64_MAX / 2) { | ||
| ms = (ms + 500) / 1000; | ||
| val_ = ms >= UINT32_MAX ? UINT32_MAX - 1 : ms; | ||
| } else { | ||
| val_ = UINT32_MAX - 1; | ||
| } |
There was a problem hiding this comment.
Not sure I understand the if with int64_t::max / 2, why not just calculate the seconds value and see if it fits into int32_t 👀 Just check to divide first and then add the remainder > 0 (I assume the if is about this)
There was a problem hiding this comment.
max64/2 is arbitrary, I could also check max64/1024. the goal is to prevent from (ms + 500) to overflow when we check if https://github.com/dragonflydb/dragonfly/pull/5797/files#diff-260e4d827ec66db63761ebe27e45792b6e2c8f9262f22d4a07e567c3fbfb5e40R61 fits uint32_t
There was a problem hiding this comment.
It's just a little less intuitive
| uint32_t val_; | ||
| uint8_t gen_ : 2; // generation id. | ||
| uint8_t precision_ : 1; // 0 - ms, 1 - sec. |
There was a problem hiding this comment.
Not sure if you're optimizing with all those bitfields, but the struct is not packed either way 🙂
There was a problem hiding this comment.
I know the memory usage in this PR does not go down. It's not the goal here. I want first to introduce all the supporting functionality to support 5 bytes expire offset and then eliminate the expire table completely by moving 5 byte expire value into PrimeValue. This will reduce the memory usage for TTL enabled use-cases but will increase it for use-cases that do not use TTLs. This is why I want to use 5 bytes for expire value so we could reduce the negative impact for these use-cases.
In order to support a 5-bytes expire offset, we must model the absolute time as base + period because we must update base from time to time, otherwise after 49 days of process being run you won't be able to represent a millisecond precision with the old base. And since updating the base needs to be done gradually (by traversing the whole table), we need to have multiple bases to support the transition state.
|
Not sure if I have to start using the phrase "not sure" less 🤔 |
We use adaptive precision, where we keep "millis" precision if we can, and switch to seconds precision if the deadline is too large. This actually was before but now I reduced the cut-off to 4B ms, or 49 days. Also, the ttl in seconds is now limited to 4B sec, with larger values quietly rounded down to this limit. Signed-off-by: Roman Gershman <[email protected]>
| uint8_t expire_allowed_ : 1; | ||
| uint8_t expire_base_index_ : 1; |
There was a problem hiding this comment.
why do you like this so much 😆
| precision_ = 1; // seconds | ||
| if (ms < UINT64_MAX / 2) { | ||
| ms = (ms + 500) / 1000; | ||
| val_ = ms >= UINT32_MAX ? UINT32_MAX - 1 : ms; | ||
| } else { | ||
| val_ = UINT32_MAX - 1; | ||
| } |
There was a problem hiding this comment.
It's just a little less intuitive
We use adaptive precision, where we keep "millis" precision if we can, and switch to seconds precision if the deadline is too large.
This actually was before but now I reduced the cut-off to 4B ms, or 49 days. Also, the ttl in seconds is now limited to 4B sec, with larger values quietly rounded down to this limit.