Skip to content

[8.0] MOD-8097, MOD-8114 Fix memory counting#5243

Merged
meiravgri merged 1 commit into8.0from
backport-5204-to-8.0
Nov 21, 2024
Merged

[8.0] MOD-8097, MOD-8114 Fix memory counting#5243
meiravgri merged 1 commit into8.0from
backport-5204-to-8.0

Conversation

@meiravgri
Copy link
Collaborator

* expose openNumericKeysDict, requires a write/read argument.

replaces all OpenNumericIndex calls.

* fork gc -initialiize index pointer only once

rename createIndex to createPopulateTermsInvIndex

add function to tests/index_utils

generalize FGCTest suit
add inheriting suite for tags

some cosmetics in cpp_range

ExtTest.testDynamicLoading: log when assert fails (the test halts upon assertion and the log will be nerver pronted)

in IndexTest and QueryTest use IndexSpec_RemoveFromGlobals instead of StrongRef_Release to ensure the spec is properly cleaned
otherwise some structs like the global prefix trie nodes, potentially accessed by following tests, might hold a copy of the spec refrence that was already released and cuase a crash.

* add inverted index total size to the numeric tree root

update in gc and in addition
add cpp_range test and cpp_forkgc test

* FIX:
decrease gc->stats.totalCollected by bytesAdded to not count bytes added by us

remove reseting currNode->range->invertedIndexSize in applyNumIdx called upon each node recieved from the child process
instead, we will count the memory released when trimming the empty leaves

call IndexSpec_TotalMemUsage from RediSearch_MemUsage instead of code duplication

add IndexSpec_collect_numeric_overhead that counts the memory of NumericRangeTree in a given  spec
call this function to add it to IndexSpec_TotalMemUsage

update LLApiTest testInfo* accordingly

* fixes

* fix new cpp forkgc test

* fix unsafe read

* fix status check

* numeric tree debug commands - if tree was not intizlied yet, reply as if it was an empty tree

* fix reset range->invertedIndexSize in applyNumIdx when index is empty.
it will be counted when the range is deleted

* add initial inverted index size if numeric to test

don't intialize the numeric inverted index in the child if it's empty

* fix gc warning

* little fix

* changes rt->invertedIndexSize to rt->invertedIndexesSize

fix IndexSpec_collect_numeric_overhead and add test

fix sending NULL to FGC_sendFixed

* CalculateNumericInvertedIndexMemory fix identation and break when failing

* rename openNumericKeysDict variable write to initialize and change type to bool

* change OPEN_INDEX_READ to DONT_CREATE_INDEX
and OPEN_INDEX_WRITE to CREATE_INDEX

add coments to tests

* use Likst instead of list to support older python versopns

* remove type from function args

* fix

---------

Co-authored-by: GuyAv46 <[email protected]>
(cherry picked from commit 357d356)
@meiravgri meiravgri enabled auto-merge November 21, 2024 17:10
@codecov
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

Attention: Patch coverage is 86.88525% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.55%. Comparing base (e693982) to head (c95b617).
Report is 1 commits behind head on 8.0.

Files with missing lines Patch % Lines
src/fork_gc.c 72.72% 6 Missing ⚠️
src/debug_commands.c 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              8.0    #5243      +/-   ##
==========================================
+ Coverage   86.54%   86.55%   +0.01%     
==========================================
  Files         192      192              
  Lines       34470    34484      +14     
==========================================
+ Hits        29831    29847      +16     
+ Misses       4639     4637       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@meiravgri meiravgri added this pull request to the merge queue Nov 21, 2024
Merged via the queue into 8.0 with commit d351abe Nov 21, 2024
@meiravgri meiravgri deleted the backport-5204-to-8.0 branch November 21, 2024 18:39
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.

2 participants