add dense_set.SetExpiryTime in preparation for fieldexpire #3780

NegatioN · 2024-09-24T11:26:46Z

Split up the previous PR #3772 to only target the updates in core as a first pass. @romange

I decided to split the dense_set.SetExpiryTime into two branching paths. One which updates the ttl directly, and one which replaces the given object, based on if we find a ttl or not.
This seems to be needed, since I can't seem to find the ttl-bits on the StringSet sds when receiving the DensePtr. (They get masked away?)

This code is the basis for adding hexpire and fieldexpire: #3027

I left a few questions in the code, and am open to other comments as well.

Edit: I haven't really landed what return types make sense for all of the functions yet, but maybe void makes the most sense for all update methods, and bool for replace methods?

romange · 2024-09-24T15:08:07Z

Thank you @NegatioN .
Seems that you need to rebase your branch - there are conflicts there.

NegatioN · 2024-09-25T08:10:06Z

Conflicts resolved @romange :)

kostasrim

Hi @NegatioN ! Thank you for your contribution 🚀

I left some comments which will reduce the size of this PR :)

Let me know if you have any questions

src/core/dense_set.cc

src/core/string_set.cc

kostasrim · 2024-09-26T14:02:17Z

src/core/dense_set.h

+      if (HasExpiry()) {
+        owner_->ObjUpdateExpireTime(curr_entry_->GetObject(), ttl_sec);
+      } else {
+        owner_->ObjReplace(curr_entry_->GetObject(), ttl_sec);


owner_->AddOrUpdate() and you can delete the function ObjReplace()

If I'm to do owner_->AddOrUpdate() I would have to expose that as a new virtual function on dense_set though? And make that function for StringSet.
Which I can obviously do, as it would remove a code-path on string_map, but it just seems like your comment assumes AddOrUpdate already exists, which confuses me a bit.

Or maybe it's because this change relies on your previous suggestion of also moving SetExpireTime into StringMap?

Sorry, I was not very clear with this -- I wanted SetExpiryTime() to be a member of the derived class and not a member of the iterator DenseSet::Iterator::. Based on Roman's comments, my suggestion is to use AddOrUpdate() here + make it virtual since Roman does not want to implement this in the derived class.

He also suggested a different approach below (see his comment below). I replied there. Let him decide out of the two approaches which one he prefers and we move on. Both of them do not require a lot of changes and they should be pretty quick to implement.

Also @NegatioN again thank you for your contribution and for your patience with this 🙏 🚀

src/core/dense_set.h

kostasrim · 2024-09-26T14:17:58Z

src/core/score_map.cc

@@ -128,6 +128,14 @@ uint32_t ScoreMap::ObjExpireTime(const void* obj) const {
  return UINT32_MAX;
 }

+void ScoreMap::ObjReplace(const void* obj, uint32_t ttl_sec) {


We can remove this :)

src/core/dense_set.h

kostasrim · 2024-09-26T14:27:34Z

src/core/string_set.cc

@@ -129,6 +115,29 @@ uint32_t StringSet::ObjExpireTime(const void* str) const {
  return absl::little_endian::Load32(ttlptr);
 }

+void StringSet::ObjUpdateExpireTime(const void* obj, uint32_t ttl_sec) {
+  DCHECK_GT(ttl_sec, 0u);  // ttl_sec == 0 would mean find and delete immediately


This is used by both StringSet && StringMap. Maybe worth extracting this into a common function and call it here to avoid code duplication ? (the only difference I see is the +8 above)

Very doable, but where would that common code live? I'm not so familiar with the code-base :)

Well this just updates the Expiry time of an sds object. A free standing function / helper that accepts a stride parameter (the + 8 in the example). Or even a function within DenseSet. (Although personally I prefer helpers to be defined outside of a class but again that's a preference not a hard requirement)

I put this in sds_util::SdsUpdateExpireTime for now. It might have implications for ObjUpdateExpireTime too, but I'll defer that change until we're in agreement on all the other points :)

src/core/string_set.cc

romange · 2024-09-26T20:52:24Z

src/core/string_map.cc

+  absl::little_endian::Store32(valptr + 8, at);
+}
+
+void StringMap::ObjReplace(const void* obj, uint32_t ttl_sec) {


I understand what you are trying to do but I think it can be simplified.

What you really want is AddExpiry semantics on the EXISTING object that DOES NOT have expiry.

Which means you already have the iterator to the object, or even stronger - the pointer to the hashtable entry/Link that contain that existing object pointer.

So you do not need to do the whole AddOrReplaceObj flow again - you already have the position (DensePtr). All you need to do is to "clone" the existing object with TTL attached to it.

Once you do that, you can swap the pointers inside DensePtr and update DensePtr tag bits to add the meta information about TTL.

Finally you will need to free the previous object pointer.

I recently added ObjectClone(const void* obj, bool has_ttl) that clones the object while keeping its ttl property as is.

You can extend its functionality to support bool add_ttl as well, so that ObjectClone(existing_obj. has_ttl=false, add_ttl=true) will clone the object that did not have ttl to the object that has (0) ttl and then you can call ObjUpdateExpireTime to update it to correct value

Two things:

I do not personally like adding (yet) another boolean in ObjectClone. One bool is fine (usually), but at the moment you start adding different bools to an interface things become less and less clear.

clone + swap is almost identical to AddOrReplace when the second behaves as a Replace.

Also, notice that the logic of ObjReplace is again almost identical to the implementation of AddOrUpdate(). Also, notice that AddOrUpdate exists in: StringMap and ScoreMap (with the former having a slightly different return type). And finally AddOrUpdate is missing from StringSet. So why don't we make AddOrUpdate a virtual function ? The only thing we need to change is a) the signature of StringMap::AddOrUpdate to return a pair (just like ScoredMap -- and it's not called in many places so the change is quick) and implement this in StringSet. Now, we can get rid ObjReplace alltogether and just use AddOrUpdate in SetExpiryTime

That way SetExpiryTime can become a member of DenseSet (and not a member of DenseSet::Iterator) + keep it in the base class to avoid duplicating in the derived classes (bonus points is also generic as you suggested).

@romange WDYT ?

P.s. not a strong opinion, just discussing my thoughts

Thanks for feedback from the both of you :)

My 2 cents: It seems like you're seeing the problem from different angles perhaps. One leaning towards code cleanliness, and the other performance.

I think AddOrUpdate isn't implemented in StringSet yet, because Update is less of a thing in a Set than it would be in StringMap or ScoreMap. And by that metric, we would probably want it to be a private method thats somehow shaded to be public for StringMap and ScoreMap if we went that route?
Also, the external public signatures can't match with the StringSet version, since there's no field+value. AddOrUpdate(std::string_view field, std::string_view value, uint32_t ttl_sec = UINT32_MAX)

I do appreciate the desire to make the code-path hit as few lines as possible, since we're only supposed to interact with already existing objects in this case. Too many flags in ObjectClone makes it a bit yucky too though.
You would also need to add an asterix about "Hey, we didn't really give you a real TTL here, so make sure to set it when you get you sds back" upon taking a closer look.

This is my best suggestion so far, which tries to keep the complexity of additional branching from add_ttl as low as possible:

void* StringSet::ObjectClone(const void* obj, bool has_ttl, bool add_ttl) const { sds src = (sds)obj; string_view sv{src, sdslen(src)}; if(add_ttl){ return (void*)MakeSetSds(sv, 0); } uint32_t at = has_ttl ? ObjExpireTime(obj) : UINT32_MAX; return (void*)MakeSetSds(sv, at); } sds StringSet::MakeSetSds(string_view src, uint32_t ttl_sec) const { DCHECK_GT(ttl_sec, 0u); // ttl_sec == 0 would mean find and delete immediately if (ttl_sec != UINT32_MAX) { uint32_t at = time_now() + ttl_sec; DCHECK_LT(time_now(), at); sds newsds = AllocImmutableWithTtl(src.size(), at); if (!src.empty()) memcpy(newsds, src.data(), src.size()); return newsds; } return sdsnewlen(src.data(), src.size()); }

Let's wait for Roman to decide what he likes the most. I do not have a strong preference :)

Yes, just jamming :)

romange · 2024-09-28T15:12:13Z

src/core/dense_set.h

@@ -187,6 +187,17 @@ class DenseSet {
      return curr_entry_->HasTtl() ? owner_->ObjExpireTime(curr_entry_->GetObject()) : UINT32_MAX;
    }

+    void SetExpiryTime(uint32_t ttl_sec) {


nit: lets move the implementation to cc file

romange

Looks good!
a small comment, and we are good to submit, I think

kostasrim · 2024-09-30T06:52:49Z

@NegatioN Test failing :)

2024-09-28T15:29:40.3732662Z 28: [ RUN      ] StringSetTest.SetFieldExpireNoHasExpiry
2024-09-28T15:29:40.3735090Z 28: F20240928 15:29:40.372570 20977 string_set.cc:134] Check failed: ttl_sec > 0u (0 vs. 0) 
2024-09-28T15:29:40.3753894Z 28: *** Check failure stack trace: ***
2024-09-28T15:29:40.3789973Z 28:     @           0x541d4d  google::LogMessage::Fail()
2024-09-28T15:29:40.3790743Z 28:     @           0x541c94  google::LogMessage::SendToLog()
2024-09-28T15:29:40.3791495Z 28:     @           0x541489  google::LogMessage::Flush()
2024-09-28T15:29:40.3792468Z 28:     @           0x5451a0  google::LogMessageFatal::~LogMessageFatal()
2024-09-28T15:29:40.3797463Z 28:     @           0x435378  dfly::StringSet::MakeSetSds()
2024-09-28T15:29:40.3804486Z 28:     @           0x43527d  dfly::StringSet::ObjectClone()
2024-09-28T15:29:40.3808152Z 28:     @           0x41e7ed  dfly::DenseSet::IteratorBase::SetExpiryTime()
2024-09-28T15:29:40.3816267Z 28:     @           0x418d49  dfly::StringSetTest_SetFieldExpireNoHasExpiry_Test::TestBody()
2024-09-28T15:29:40.3823567Z 28:     @           0x4b3604  testing::internal::HandleSehExceptionsInMethodIfSupported<>()
2024-09-28T15:29:40.3828655Z 28:     @           0x4abe81  testing::internal::HandleExceptionsInMethodIfSupported<>()
2024-09-28T15:29:40.3833348Z 28:     @           0x486a94  testing::Test::Run()
2024-09-28T15:29:40.3838515Z 28:     @           0x487563  testing::TestInfo::Run()
2024-09-28T15:29:40.3841551Z 28:     @           0x487f61  testing::TestSuite::Run()
2024-09-28T15:29:40.3849318Z 28:     @           0x497f00  testing::internal::UnitTestImpl::RunAllTests()
2024-09-28T15:29:40.3855626Z 28:     @           0x4b47a7  testing::internal::HandleSehExceptionsInMethodIfSupported<>()
2024-09-28T15:29:40.3859418Z 28:     @           0x4ad195  testing::internal::HandleExceptionsInMethodIfSupported<>()
2024-09-28T15:29:40.3867055Z 28:     @           0x4964fc  testing::UnitTest::Run()

NegatioN · 2024-09-30T07:11:50Z

@kostasrim , I think that must be a test-run from a "not-most-recent" commit? String set seems fine when I compile and run the most recent one.

NegatioN · 2024-09-30T07:24:09Z

I also fixed the nit @romange :) I haven't really cleaned up the commits, because I figured this should probably just be squashed.

This commit is in preparation for adding FIELDEXPIRE and HEXPIRE.

NegatioN · 2024-10-01T07:15:23Z

Not to nag, but as soon as this is merged, I can prepare the next PR for adding Fieldexpire and Hexpire 🙂

kostasrim · 2024-10-01T07:43:59Z

Not to nag, but as soon as this is merged, I can prepare the next PR for adding Fieldexpire and Hexpire 🙂

units are failing can you plz check ?

NegatioN · 2024-10-01T07:50:11Z

@kostasrim Sorry about that, it seems like I misunderstood the significance of DCHECK_GT. Zero is currently a valid input for MakeSetSds (when cloning a object with unset TTL), but I had a check there that demanded it to be GT(0).

Please try rerunning the suite.

romange · 2024-10-01T08:21:46Z

Yeah, I suggest you build in debug mode locally to catch these things

NegatioN · 2024-10-01T08:35:25Z

@kostasrim For future reference, I know this may seem very basic, but how do I actually build this in debug mode? I've mainly used ninja $task, ninja src/all and ctest -V -L DFLY
This is kinda my first contribution to a c++ project, so the tooling is a bit unfamiliar.

NegatioN force-pushed the core-updates branch 2 times, most recently from 72c0eed to 125add6 Compare September 24, 2024 12:52

romange added hacktoberfest Participates in Hacktoberfest hacktoberfest-accepted labels Sep 24, 2024

NegatioN force-pushed the core-updates branch from 125add6 to e11c3b7 Compare September 25, 2024 08:07

NegatioN marked this pull request as ready for review September 25, 2024 08:08

romange requested a review from adiholden September 25, 2024 08:43

NegatioN force-pushed the core-updates branch from 5391b26 to 2b31a65 Compare September 25, 2024 10:17

kostasrim reviewed Sep 26, 2024

View reviewed changes

romange reviewed Sep 26, 2024

View reviewed changes

NegatioN force-pushed the core-updates branch from 00c02a2 to 38ec7aa Compare September 27, 2024 14:07

romange reviewed Sep 28, 2024

View reviewed changes

feat: add DenseSet::IteratorBase::SetExpiryTime

7f2dbce

This commit is in preparation for adding FIELDEXPIRE and HEXPIRE.

NegatioN force-pushed the core-updates branch from 5afeca0 to 7f2dbce Compare September 30, 2024 11:21

Merge branch 'main' into core-updates

a3fadb8

fix: 0 is a valid input for MakeSetSds

969a24f

NegatioN force-pushed the core-updates branch from 2336d16 to 969a24f Compare October 1, 2024 07:55

romange approved these changes Oct 1, 2024

View reviewed changes

romange enabled auto-merge (squash) October 1, 2024 08:27

romange merged commit 2ab480e into dragonflydb:main Oct 1, 2024
9 checks passed

NegatioN deleted the core-updates branch October 1, 2024 10:02

NegatioN mentioned this pull request Oct 1, 2024

feat: add HEXPIRE and FIELDEXPIRE #3842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add dense_set.SetExpiryTime in preparation for fieldexpire #3780

add dense_set.SetExpiryTime in preparation for fieldexpire #3780

NegatioN commented Sep 24, 2024 •

edited

Loading

romange commented Sep 24, 2024

NegatioN commented Sep 25, 2024

kostasrim left a comment

kostasrim Sep 26, 2024

NegatioN Sep 26, 2024 •

edited

Loading

kostasrim Sep 27, 2024

kostasrim Sep 26, 2024

kostasrim Sep 26, 2024 •

edited

Loading

NegatioN Sep 26, 2024

kostasrim Sep 27, 2024 •

edited

Loading

NegatioN Sep 27, 2024

romange Sep 26, 2024 •

edited

Loading

romange Sep 26, 2024

romange Sep 26, 2024

kostasrim Sep 27, 2024 •

edited

Loading

NegatioN Sep 27, 2024 •

edited

Loading

NegatioN Sep 27, 2024

kostasrim Sep 27, 2024

NegatioN Sep 27, 2024

romange Sep 28, 2024

romange left a comment

kostasrim commented Sep 30, 2024

NegatioN commented Sep 30, 2024

NegatioN commented Sep 30, 2024

NegatioN commented Oct 1, 2024

kostasrim commented Oct 1, 2024

NegatioN commented Oct 1, 2024

romange commented Oct 1, 2024

NegatioN commented Oct 1, 2024

add dense_set.SetExpiryTime in preparation for fieldexpire #3780

add dense_set.SetExpiryTime in preparation for fieldexpire #3780

Conversation

NegatioN commented Sep 24, 2024 • edited Loading

romange commented Sep 24, 2024

NegatioN commented Sep 25, 2024

kostasrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NegatioN Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostasrim Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

NegatioN Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange left a comment

Choose a reason for hiding this comment

kostasrim commented Sep 30, 2024

NegatioN commented Sep 30, 2024

NegatioN commented Sep 30, 2024

NegatioN commented Oct 1, 2024

kostasrim commented Oct 1, 2024

NegatioN commented Oct 1, 2024

romange commented Oct 1, 2024

NegatioN commented Oct 1, 2024

NegatioN commented Sep 24, 2024 •

edited

Loading

NegatioN Sep 26, 2024 •

edited

Loading

kostasrim Sep 26, 2024 •

edited

Loading

kostasrim Sep 27, 2024 •

edited

Loading

romange Sep 26, 2024 •

edited

Loading

kostasrim Sep 27, 2024 •

edited

Loading

NegatioN Sep 27, 2024 •

edited

Loading