Conversation
romange
force-pushed
the
Pr1
branch
2 times, most recently
from
d711ad0 to
3e90099
Compare
| OpResult<bool> OpSet(const OpArgs& op_args, string_view key, string_view path, | ||
| const WrappedJsonPath& json_path, std::string_view json_str, | ||
| bool is_nx_condition, bool is_xx_condition) { | ||
| JsonMemTracker mem_tracker; |
There was a problem hiding this comment.
Since this is not really used below I would rather have it local to the scope of the if statement below but IMO it doesn't really matter.
There was a problem hiding this comment.
It is used. mem_tracker tracks used memory and ShardJsonFromString allocates from it
There was a problem hiding this comment.
I mean, that mem_tracker is local to the branch below and it's only used there and not in other places within this function.
See line 1311 mem_tracker.SetJsonSize(st->it->second, st->is_new); and after it we return.
Pardon me, I wasn't very clear 😄
| // is to use absl::Cleanup and dispatch another Find() but that's too complicated because then | ||
| // you need to take into account the order of destructors. | ||
| OpResult<DbSlice::AddOrFindResult> st = SetJson(op_args, key, parsed_json.value()); | ||
| OpResult<DbSlice::AddOrFindResult> st = SetJson(op_args, key, std::move(*parsed_json)); |
There was a problem hiding this comment.
This is fine, because:
- We pass a lambda now
parse_json. See changes introduced by Stefan in fb8234c#diff-0799c8810dbf8758f9da40162075f38bb51639f806be28a06d9f669a6ebb0f76R1288
(The only redundancy here is the move on the lambda since it has no capture -- but I know you like calling move() anyways so it's fine).
- Moving the JsonMemTracker above is also fine. We no longer parse the json in this function but rather inside the lambda so when we construct the tracker we have no extra calculated allocations and when we call
mem_tracker.SetJsonSize(st->it->second, st->is_new);below we already destructed the temporaries created withinSetJson()from the lambdaparse_jsonso the tracking deltas shall becorrect
romange
dismissed stale reviews from kostasrim and BagritsevichStepan
via
18be08c
|
Signed-off-by: Roman Gershman <roman@dragonflydb.io>
|
I removed the lambda |
| VLOG(1) << "got invalid JSON string '" << json_str << "' cannot be saved"; | ||
| return OpStatus::SYNTAX_ERR; | ||
| } | ||
| return parsed_json.value(); |
There was a problem hiding this comment.
this does not move, this copies.
BagritsevichStepan
left a comment
BagritsevichStepan
left a comment
There was a problem hiding this comment.
I'd like to suggest running sanitizers on this branch
|
Upd.: Still fails |
3 participants
jsoncons 0.178 has some pmr related fixes that should contribute to better stability in our codebase.