Skip to content

fix memory leak on incor calc of cleanup bucket#429

Merged
mangalaman93 merged 3 commits intodgraph-io:mainfrom
flymedllva:fix-memory-leak-on-incor-calc-of-cleanup-bucket
Dec 31, 2024
Merged

fix memory leak on incor calc of cleanup bucket#429
mangalaman93 merged 3 commits intodgraph-io:mainfrom
flymedllva:fix-memory-leak-on-incor-calc-of-cleanup-bucket

Conversation

@flymedllva
Copy link
Contributor

@flymedllva flymedllva commented Dec 26, 2024 •
edited
Loading

cleanup now does not try to clean an empty bucket selected by the cleanup algorithm

this fixes a critical problem of slice growth when the algorithm incorrectly calculates the buckets that need to be cleared, let's say this behavior occurs at the time change described in issue: #426

this problem appeared after improving cleanup in this pr: #358

most likely the algorithm for calculating the cleaning bins also needs to be revised, but for me now it is important to fix the increase in memory consumption when changing the system time

@flymedllva flymedllva requested a review from a team December 26, 2024 00:16
mangalaman93
mangalaman93 previously approved these changes Dec 26, 2024
View reviewed changes
@mangalaman93 mangalaman93 enabled auto-merge (squash) December 26, 2024 05:39
@flymedllva flymedllva force-pushed the fix-memory-leak-on-incor-calc-of-cleanup-bucket branch from 0507668 to 4b34a22 Compare December 27, 2024 10:08
@mangalaman93
Copy link
Contributor

mangalaman93 commented Dec 27, 2024

@flymedllva Thank you for the PR. The ci passes. is it possible to write a test for the issue?

@flymedllva
fix memory leak on incor calc of cleanup bucket
d4417de 
cleanup does not try to clean empty bucket
@flymedllva flymedllva force-pushed the fix-memory-leak-on-incor-calc-of-cleanup-bucket branch from c61ba03 to 89d19c9 Compare December 27, 2024 21:42
@flymedllva
Copy link
Contributor Author

flymedllva commented Dec 27, 2024

I couldn't find a direct test for cleanup, so I made one and included in its implementation a sub-check that caused the slice to grow uncontrollably without allowing cleanup to terminate. With the fix, this test passes.

@flymedllva flymedllva force-pushed the fix-memory-leak-on-incor-calc-of-cleanup-bucket branch from 89d19c9 to dc92bd6 Compare December 27, 2024 21:58
mangalaman93
mangalaman93 previously approved these changes Dec 29, 2024
View reviewed changes
@mangalaman93
Copy link
Contributor

mangalaman93 commented Dec 29, 2024

The test seems to fail:

--- FAIL: TestExpirationMapCleanup (5.00s)
    --- FAIL: TestExpirationMapCleanup/Miscalculation_of_buckets_does_not_cause_memory_leaks (1.00s)
        ttl_test.go:83: 
            	Error Trace:	/home/runner/work/ristretto/ristretto/ttl_test.go:83
            	Error:      	cleanup method hangs / there may be a memory leak!
            	Test:       	TestExpirationMapCleanup/Miscalculation_of_buckets_does_not_cause_memory_leaks
FAIL

@flymedllva
Copy link
Contributor Author

flymedllva commented Dec 29, 2024

Apparently CI runners have fewer CPUs and because of this the tests take longer than on my computer. To fix this I had to introduce a cleanedBucketsCount variable in cleanup return, which I can use in the test to count the buckets cleaned by cleanup.

The test is no longer CPU-dependent and takes less time to run + it catches this case immediately during the first cleanup without reaching the test case with system time change (although it works too).

@mangalaman93
Copy link
Contributor

mangalaman93 commented Dec 29, 2024

Thank you for updating the PR. The linter is not happy now:

Running [/home/runner/golangci-lint-1.62.2-linux-amd64/golangci-lint run --new-from-patch=/tmp/tmp-1914-PGn62ZkvM864/pull.patch --new=false --new-from-rev= --timeout=10m] in [/home/runner/work/ristretto/ristretto] ...
  Received 174177 of 174177 (100.0%), 0.2 MBs/sec
  ttl_test.go:72: the line is 124 characters long, which exceeds the maximum of 120 characters. (lll)
  		require.Equal(t, 0, cleanedBucketsCount, "cleanedBucketsCount should be 0 after cleanup with lastCleanedBucketNum change")

@flymedllva flymedllva force-pushed the fix-memory-leak-on-incor-calc-of-cleanup-bucket branch from ccc15e4 to daf48d2 Compare December 29, 2024 19:57
@mangalaman93 mangalaman93 merged commit 65472b1 into dgraph-io:main Dec 31, 2024
@mangalaman93
Copy link
Contributor

mangalaman93 commented Dec 31, 2024

Thank you so much for the quick fix and addressing all the issues. Appreciate it.

@mangalaman93 mangalaman93 mentioned this pull request Jan 9, 2025
Closed
@fnoehWM fnoehWM mentioned this pull request Oct 20, 2025
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mangalaman93 mangalaman93 mangalaman93 approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@flymedllva @mangalaman93