JSON Module rewrite with custom JSONPath implementation #974
Conversation
|
Edit: Ran |
|
@darrenge Add expected values in BDN_Benchmark_Config.json |
There was a problem hiding this comment.
What an extensive effort! Nicely done @Vijay-Nirmal. I've added some comments. I haven't looked deeply into the unmitigated issues yet (custom writer & null sets), I'll look at those next and give my opinion...
|
Thoughts about INDENT / NEWLINE / SPACE...
|
|
@TalZaccai I have fixed the review comments
I don't think rewriting a
I didn't get this. Can you elaborate a bit more? I didn't get what you mean by "JSON string"
We can support these params in future, I have some ideas but need some complex work. Will do it as part of a separate PR. I don't recommend returning an error because the current implementation uses a default format to format the output JSON when any of these parameters is passed which has helped me in readying the output. Without any format, the output is very hard to read. Let's get this initial rewrite of the JSON module PR go in then we can do these as part of enhancements.
Basically, write a semi-custom JSON writer using Utf8jsonwriter. |
| /// <param name="offset">The current offset in the input arguments.</param> | ||
| /// <param name="value">The parsed expire option if successful, otherwise <see cref="ExistOptions.None"/>.</param> | ||
| /// <returns>True if the expire option was successfully parsed, otherwise false.</returns> | ||
| public static bool TryGetExpireOption(this ref ObjectInput input, scoped ref int offset, out ExistOptions value) |
There was a problem hiding this comment.
naming: Should be TryGetExistOption (also in the comment)
There was a problem hiding this comment.
nit: XML comment was not updated (still says expire option)
| { | ||
| if (rootNode is null) | ||
| { | ||
| errorMessage = default; |
|
@Vijay-Nirmal - just wanted to check - where are we on getting this PR to a point where it is complete and ready as a module to merge? |
|
@badrishc I will fix the review comments soon, I didn't get the review comment notification till yesterday for this |
|
Fixed all review remaining comments @badrishc @TalZaccai |
There was a problem hiding this comment.
LGTM. One last thing that I'd like to see Vijay -
If you could take the JSON benchmarks (Operations.JsonOperations) and run them against main (probably just create a branch off latest main and add only the changes to BDN.Benchmark), then compare those results to results from this branch and post these here, that would be awesome!
|
@TalZaccai Already done that and posted in the original post under "Garnet BDN Benchmark" section |
Oh cool, I missed that in the slough of benchmarks! :) Can we get an updated one though (most recent main vs. most recent version of this PR)? Appreciate it!! |
| @@ -682,7 +682,7 @@ public void CustomObjectCommandTest4() | |||
| server.Register.NewCommand("MYDICTGET", CommandType.Read, factory, new MyDictGet(), new RespCommandsInfo { Arity = 3 }); | |||
|
|
|||
| // Register sample custom command on object 2 | |||
| var jsonFactory = new JsonObjectFactory(); | |||
| var jsonFactory = new GarnetJsonObjectFactory(); | |||
There was a problem hiding this comment.
We need unit tests that load the entire JSON module to the server, then runs JSON commands on such a server. These tests are simply registering and loading specific commands directly.
Please test with:
- MODULE LOADCS for dynamic module loading
- server.Register.NewModule() as an option when creating the test server
There was a problem hiding this comment.
@Vijay-Nirmal you can use RespModuleTests.TestNoOpModule for reference on loading a module programmatically / from a pre-built dll.
|
Why is ModuleJsonGetCommand allocating more on the PR compared to main? |
|
Also, please move NoOpModule to the modules folder for consistency, and update any reference to it, e.g. in tests or docs. |
Looks like MemorySteam is not playing well for smaller results. Most of the memory allocation issues go away if I hardcode a small capacity for benchmarking. Of course, we can't do that, so I changed the implementation to use List<byte[]> to hold the output, which improves the memory significantly, but for the very basic get benchmark, there is a slight increase in the mean time (+5 to +10us variance between runs). Personally not a fan of this implementation as the implementation looks not good. Will see if I can do something else. @badrishc @TalZaccai If you guys have some idea, then let me know Main branch (Large run to run variance for complex methods)
PR (Large run to run variance for complex methods)
|
As part of this PR, the JSON module implementation has been rewritten using a custom implementation of JsonPath to achieve better performance and compatibility with Redis. As part of this rewrite other parts of the Json module have also been rewritten
Main Changes:
JsonPath.Netpackage referenceMiscellaneous:
CmdStringsclass public inCmdStrings.csto allow external access.ExistOptionsenum to public inRespEnums.csfor broader accessibility.Known Issues:
nullusingJSON.SETcommand then the update will fail, it can't find the parent object to update the value. This is a limitation of the custom JSONPath implementation because of the way the Null is represented in JsonNode. Will find a way to support this in future without affecting the performance much. This issue shouldn't block the PR from being merged as this scenario was failing in the current implementation as well.Utf8JsonWriterdoesn't allow enough customizability to supportINDENT,NEWLINEandSPACEoptions in JSON.GET command. We can't inherit the Utf8JsonWriter class as well as it's a sealed class to write our own logic to customize it. This feature will not be supported in STJ in future as well [API Proposal]: Allow inheritance for Utf8JsonWriter class dotnet/runtime#111899 (comment). We have only two options, either Garnet can never support the customizability provided by Redis for certain JSON commands, or Garnet should use Newtonsoft.Json (which supports customisation). Both of these options are not ideal.TODO
Custom JSONPath Benchmark
Note and a disclaimer: JsonCraft.JsonPath is a package I published. If you need Garent's implementation as a separate package you can use JsonCraft.JsonPath package. In the benchmark, it looks slightly slower because it's benchmarked against the
JsonElementimplementation instead ofJsonNode. You can find the exact same implementations as Garent in Experimental folderGarnet BDN Benchmark
In the main branch as of 762a9d7
In this PR as of 7650f5f: (More improvements to come)
Out of scope of this PR
Some of these items I will be raising future PRs to address it
/