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

Fix assertion failure and memory leak in ClockCache. #10430

Closed
wants to merge 3 commits into from

Conversation

guidotag
Copy link
Contributor

@guidotag guidotag commented Jul 27, 2022

Summary: This fixes two issues:

  • T127355728: In the stress tests, when the ClockCache is operating close to full capacity and a burst of inserts are concurrently executed, every slot in the hash table may become occupied. This contradicts an assertion in the code, which is no longer valid in the lock-free setting. We are removing that assertion and handling the case of an insertion into a full table.
  • T127427659: There was a memory leak when an insertion is performed over capacity, but no handle is provided. In that case, a handle was dynamically allocated, but the pointer wasn't stored anywhere.

Test plan:

  • make -j24 check
  • make -j24 USE_CLANG=1 COMPILE_WITH_ASAN=1 COMPILE_WITH_UBSAN=1 CRASH_TEST_EXT_ARGS="--duration=960 --cache_type=clock_cache" blackbox_crash_test_with_atomic_flush
  • make -j24 USE_CLANG=1 COMPILE_WITH_TSAN=1 CRASH_TEST_EXT_ARGS="--duration=960 --cache_type=clock_cache" blackbox_crash_test_with_atomic_flush

@facebook-github-bot
Copy link
Contributor

@guidotag has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

Better title & summary for a public audience, please. Someone should be able to quickly decide whether this change is something they should care about or affects something they are wondering about. If it's marked an assertion failure in the experimental ClockCache, we can save people time.

if (handle != nullptr) {
h = DetachedInsert(&tmp);
} else {
s = Status::MemoryLimit(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure LRUCache uses OK for this kind of case, and therefore we should also.

// Don't insert the entry but still return ok, as if the entry inserted

@facebook-github-bot
Copy link
Contributor

@guidotag has updated the pull request. You must reimport the pull request before landing.

Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@guidotag guidotag changed the title Fix assertion failure. Fix assertion failure and memory leak in ClockCache. Jul 27, 2022
@facebook-github-bot
Copy link
Contributor

@guidotag has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants