Skip to content

Commit f0faf6f

Browse files
committed
server(JSON): Remove parse method which used default heap
There were two parse methods, one which used the default heap and the other which used a thread local mi heap. The former one is removed here. The reason is to prepare for using a stateless allocator. The allocator is part of the type of the json object. A stateless allocator must not hold state of its own and simply forward requests to a memory resource pointer accessed from a static storage. In such a model, having two memory resources does not work because two allocators using different heaps are not interchangeable. It is possible to use two different types, but in that case the json object types become different, and it becomes impossible to work with them transparently. The switch to mi heap everywhere can cause one problem during accounting. The JSONAutoUpdater class uses memory usage before and after parsing to figure out how many bytes are used by a given object. If the objects parsed by the method which is removed in this PR are interspersed with JSONAutoUpdater, then this can cause a problem with accounting. At present there are no such cases in the code. Signed-off-by: Abhijat Malviya <[email protected]>
1 parent 6899751 commit f0faf6f

File tree

1 file changed

+10
-15
lines changed

1 file changed

+10
-15
lines changed

src/server/json_family.cc

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -470,17 +470,12 @@ std::optional<std::string> ConvertJsonPathToJsonPointer(string_view json_path) {
470470
return pointer;
471471
}
472472

473-
// Use this method on the coordinator thread
474-
std::optional<JsonType> JsonFromString(std::string_view input) {
475-
return dfly::JsonFromString(input, PMR_NS::get_default_resource());
476-
}
477-
478-
/* Use this method on the shard thread
473+
/*
479474
480475
If you do memory tracking, make sure to initialize it before calling this method, and reset the
481476
result before invoking SetJsonSize. Note that even after calling std::move on an optional, it may
482477
still hold the JSON value, which can lead to incorrect memory tracking. */
483-
std::optional<JsonType> ShardJsonFromString(std::string_view input) {
478+
std::optional<JsonType> ParseJSONFromString(std::string_view input) {
484479
return dfly::JsonFromString(input, CompactObj::memory_resource());
485480
}
486481

@@ -501,7 +496,7 @@ OpStatus SetFullJson(const OpArgs& op_args, string_view key, string_view json_st
501496
JsonAutoUpdater updater(op_args, key, *std::move(it_res),
502497
{.disable_indexing = true, .update_on_delete = false});
503498

504-
std::optional<JsonType> parsed_json = ShardJsonFromString(json_str);
499+
std::optional<JsonType> parsed_json = ParseJSONFromString(json_str);
505500
if (!parsed_json) {
506501
VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
507502
if (type == OBJ_JSON) {
@@ -548,7 +543,7 @@ OpResult<bool> SetPartialJson(const OpArgs& op_args, string_view key,
548543
The reason being, that we are applying this multiple times for each match we found.
549544
So for example if we have an array that this expression will match each entry in it then the
550545
assign here is called N times. */
551-
std::optional<JsonType> parsed_json = ShardJsonFromString(json_str);
546+
std::optional<JsonType> parsed_json = ParseJSONFromString(json_str);
552547
if (!parsed_json) {
553548
VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
554549
return OpStatus::INVALID_JSON;
@@ -845,7 +840,7 @@ OpResult<std::string> OpJsonGet(const OpArgs& op_args, string_view key,
845840
} else if (it->second.ObjType() == OBJ_STRING) {
846841
string tmp;
847842
it->second.GetString(&tmp);
848-
auto parsed_json = ShardJsonFromString(tmp);
843+
auto parsed_json = ParseJSONFromString(tmp);
849844
if (!parsed_json) {
850845
return OpStatus::WRONG_TYPE;
851846
}
@@ -1586,7 +1581,7 @@ OpStatus OpMSet(const OpArgs& op_args, const ShardArgs& args) {
15861581
// implemented yet.
15871582
OpStatus OpMerge(const OpArgs& op_args, string_view key, string_view path,
15881583
const WrappedJsonPath& json_path, std::string_view json_str) {
1589-
std::optional<JsonType> parsed_json = ShardJsonFromString(json_str);
1584+
std::optional<JsonType> parsed_json = ParseJSONFromString(json_str);
15901585
if (!parsed_json) {
15911586
VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved";
15921587
return OpStatus::INVALID_JSON;
@@ -1826,7 +1821,7 @@ void JsonFamily::ArrIndex(CmdArgList args, const CommandContext& cmd_cntx) {
18261821
auto* builder = static_cast<RedisReplyBuilder*>(cmd_cntx.rb);
18271822
WrappedJsonPath json_path = GET_OR_SEND_UNEXPECTED(ParseJsonPath(path));
18281823

1829-
optional<JsonType> search_value = JsonFromString(parser.Next());
1824+
optional<JsonType> search_value = ParseJSONFromString(parser.Next());
18301825
if (!search_value) {
18311826
builder->SendError(kSyntaxErr);
18321827
return;
@@ -1875,7 +1870,7 @@ void JsonFamily::ArrInsert(CmdArgList args, const CommandContext& cmd_cntx) {
18751870

18761871
vector<JsonType> new_values;
18771872
for (size_t i = 3; i < args.size(); i++) {
1878-
optional<JsonType> val = JsonFromString(ArgS(args, i));
1873+
optional<JsonType> val = ParseJSONFromString(ArgS(args, i));
18791874
if (!val) {
18801875
builder->SendError(kSyntaxErr);
18811876
return;
@@ -1905,7 +1900,7 @@ void JsonFamily::ArrAppend(CmdArgList args, const CommandContext& cmd_cntx) {
19051900
// TODO: there is a bug here, because we parse json using the allocator from
19061901
// the coordinator thread, and we pass it to the shard thread, which is not safe.
19071902
for (size_t i = 2; i < args.size(); ++i) {
1908-
optional<JsonType> converted_val = JsonFromString(ArgS(args, i));
1903+
optional<JsonType> converted_val = ParseJSONFromString(ArgS(args, i));
19091904
if (!converted_val) {
19101905
builder->SendError(kSyntaxErr);
19111906
return;
@@ -2000,7 +1995,7 @@ void JsonFamily::StrAppend(CmdArgList args, const CommandContext& cmd_cntx) {
20001995
WrappedJsonPath json_path = GET_OR_SEND_UNEXPECTED(ParseJsonPath(path));
20011996

20021997
// We try parsing the value into json string object first.
2003-
optional<JsonType> parsed_json = JsonFromString(value);
1998+
optional<JsonType> parsed_json = ParseJSONFromString(value);
20041999
if (!parsed_json || !parsed_json->is_string()) {
20052000
return builder->SendError("expected string value", kSyntaxErrType);
20062001
};

0 commit comments

Comments
 (0)