fix(json_family): Fix json memory tracking issues. SECOND PR #5056
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fixes dragonflydb#5055 , dragonflydb#5054 Signed-off-by: Stepan Bagritsevich <[email protected]>
2 tasks
BagritsevichStepan
changed the title
romange
reviewed
May 5, 2025
| @@ -2060,6 +2111,8 @@ void JsonFamily::Register(CommandRegistry* registry) { | |||
| *registry << CI{"JSON.ARRAPPEND", CO::WRITE | CO::DENYOOM | CO::FAST, -4, 1, 1, acl::JSON}.HFUNC( | |||
| ArrAppend); | |||
| *registry << CI{"JSON.ARRINDEX", CO::READONLY | CO::FAST, -4, 1, 1, acl::JSON}.HFUNC(ArrIndex); | |||
| *registry << CI{"JSON._MEMUSAGE", CO::READONLY | CO::FAST, 2, 1, 1, acl::JSON}.HFUNC( | |||
There was a problem hiding this comment.
you do not need it, we already have "memory usage " that should work for json as well.
There was a problem hiding this comment.
As I understand, MEMORY USAGE also includes the size of the key, which might be an issue in this case. I think it would be useful to add a flag to the MEMORY USAGE command that excludes the key size from the calculation.
romange
reviewed
May 5, 2025
| if (update_on_delete_ && !set_size_was_called_) { | ||
| SetJsonSize(); | ||
| } else if (!set_size_was_called_) { | ||
| VLOG(1) << "JsonAutoUpdater destructor called without SetJsonSize() being called. This may " |
There was a problem hiding this comment.
It shouldn't fail in debug. For example:
JsonAutoUpdater updater{};
...
if (<result is not valid>) {
return;
}
If the result is not valid we will not update the json. This means we don't need to call SetJsonSize, and that's totally fine. I thought we could use LOG(WARNING) here.
romange
reviewed
May 5, 2025
|
Really good work, @BagritsevichStepan ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes a lot of json memory tracking issues, #5055 , #5054 .
Also, partially fixes #4926
Bugs that were found and fixed:
ShardJsonFromString, which parses the json_str into the jsoncons's json and returns optional of this. The bug was thatstd::optionalstill holds the value even afterstd::move. So we overestimate the memory usage.JSON.SETwhen we calledShardJsonFromStringbefore starting the memory trackingop_args.shard->search_indices()->RemoveDocafter starting the memory tracking.RemoveDochas a static cache that may allocate some memory, but this memory does not belong to the JSON object itself. As a result, memory usage will be overestimated in this case.JSON.DELcommand - we didn't update the indexes and track the memory usage at all. One of the bugs is good described in JSON value is not updated in the indexes after the JSON.DEL command #5055JSON.SETcommand: JSON value is removed from the index after a failed JSON.SET command #5054To fix this, I introduced
json_family_memory_testand changedJsonMemTrackertoJsonAutoUpdater, which eliminates most of the bugs described above.It also ensures that RemoveDoc is called before memory tracking starts, and that AddDoc is called after SetJsonSize and also it will be called even if an error occurs.
Therefore, whenever we perform an update operation on a JSON value, this class must be used.