Skip to content
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

DenseSet::Grow optimization #3894

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

BorysTheDev
Copy link
Contributor

@BorysTheDev BorysTheDev commented Oct 9, 2024

fixes: #3870

Before BM_Grow 721975 ns 721914 ns 1020
Now BM_Grow 598066 ns 597285 ns 1237

@BorysTheDev BorysTheDev force-pushed the DenseSet_Grow_optimization branch 4 times, most recently from 6a3f8e8 to 990fc48 Compare October 14, 2024 16:43
@BorysTheDev BorysTheDev changed the title DenseSet::Grow optimization (not ready for review) DenseSet::Grow optimization Oct 14, 2024
@BorysTheDev BorysTheDev marked this pull request as ready for review October 14, 2024 17:44
@romange
Copy link
Collaborator

romange commented Oct 25, 2024

TLDR: on my machine, the original code:

M_Grow       7003864 ns      7003556 ns          795

This PR:

BM_Grow       7088574 ns      7088288 ns          785

In more detail:

  1. Benchmark. I found this one to be more robust. The reason - Fill() is more efficient than AddMany.
    In addition, adding an element at size 2^k does not guarantee trigerring resize so I added explicit checks to make sure.
void BM_Grow(benchmark::State& state) {
  vector<string> strs;
  mt19937 generator(0);
  StringSet src;
  unsigned elems = 1 << 18;
  for (size_t i = 0; i < elems; ++i) {
    src.Add(random_string(generator, 16), UINT32_MAX);
    strs.push_back(random_string(generator, 16));
  }

  while (state.KeepRunning()) {
    state.PauseTiming();
    StringSet tmp;
    src.Fill(&tmp);
    CHECK_EQ(tmp.BucketCount(), elems);
    state.ResumeTiming();
    for (const auto& str : strs) {
      tmp.Add(str);
      if (tmp.BucketCount() > elems) {
        break;  // we grew
      }
    }

    CHECK_GT(tmp.BucketCount(), elems);
  }
}
BENCHMARK(BM_Grow);
  1. I actually tried to optimize as well and I got even worse results than this PR. I must say the base line of Grow is already very good but I would like some more time to analyze this.

@romange
Copy link
Collaborator

romange commented Oct 25, 2024

And I still think that cached links is unnecessary over complication.

@BorysTheDev
Copy link
Contributor Author

And I still think that cached links is unnecessary over complication.

We already use FreeLink and NewLink, we can create a small class that incapsulate this logic and cached links. it's very smooth and really good for performance even for ADD/REM operations

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.

Optimize DenseSet::Grow
2 participants