Skip to content

chore: add defrag for StringSet#4308

Merged
kostasrim merged 2 commits intomainfrom
kpr1
Jan 27, 2025
Merged

chore: add defrag for StringSet#4308
kostasrim merged 2 commits intomainfrom
kpr1

Conversation

@kostasrim
Copy link
Copy Markdown
Contributor

  • add defrag logic for StringSet
  • add test

Resolves #3822

@kostasrim kostasrim self-assigned this Dec 13, 2024
};

for (size_t i = 0; i < 10'000; i++)
ss_->Add(build_str(i));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would maybe worth to add a unit with ttl to be 100% sure we copy it properly. So two tests in total:

  1. As is now, without ttl
  2. With ttl

Comment thread src/core/string_set.cc
}

// Does not release obj. Callers must deallocate with sdsfree explicitly
pair<sds, bool> StringSet::DuplicateEntryIfFragmented(void* obj, float ratio) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once I am back from the holidays I will see how to refactor those under one single function for all DenseSet variants. Furthermore, same applies for the tests

@romange
Copy link
Copy Markdown
Collaborator

romange commented Dec 14, 2024

It can wait for your return, I prefer not to merge it now

@kostasrim kostasrim requested review from adiholden and chakaz January 7, 2025 08:00
@adiholden adiholden requested review from romange and removed request for adiholden and chakaz January 8, 2025 10:31
@kostasrim kostasrim requested review from chakaz and removed request for romange January 20, 2025 08:26
Copy link
Copy Markdown
Contributor

@chakaz chakaz left a comment

Choose a reason for hiding this comment

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

Generally LG, but I'd love @romange to at least take a look at the ReallocIfNeeded part. Specifically is it always safe and true to do:

  auto* ptr = curr_entry_;
  if (ptr->IsLink()) {
    ptr = ptr->AsLink();
  }

  DCHECK(!ptr->IsEmpty());
  DCHECK(ptr->IsObject());

?

@kostasrim
Copy link
Copy Markdown
Contributor Author

kostasrim commented Jan 23, 2025

Generally LG, but I'd love @romange to at least take a look at the ReallocIfNeeded part. Specifically is it always safe and true to do:

  auto* ptr = curr_entry_;
  if (ptr->IsLink()) {
    ptr = ptr->AsLink();
  }

  DCHECK(!ptr->IsEmpty());
  DCHECK(ptr->IsObject());

?

Yep this is fine, links must be converted via AsLink to properly use. We do this in other places well, see for example: StringMap::ReallocIfNeeded.

In fact, we can unify some of those functions under the same interface -- something I wanted to do but I do not currently have the bandwidth for the small refactor

@kostasrim kostasrim merged commit fbd785c into main Jan 27, 2025
@kostasrim kostasrim deleted the kpr1 branch January 27, 2025 10:53
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.

Implement defragmentation for OBJ_ZSET and OBJ_SET

3 participants