New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JSON serialization performance improvement #1527
JSON serialization performance improvement #1527
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just couple of remarks,, looks like a great improvement with just so little code changes!
Jint/Native/Json/JsonSerializer.cs
Outdated
@@ -303,63 +301,110 @@ private static bool CanSerializesAsArray(ObjectInstance value) | |||
/// <summary> | |||
/// https://tc39.es/ecma262/#sec-quotejsonstring | |||
/// </summary> | |||
private static void QuoteJSONString(string value, ref SerializerState target) | |||
#if NETCOREAPP1_0_OR_GREATER | |||
[MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be easier to just use value 512
without #if
here as it's (I think) a common practice, casting it to MethodImplOptions
should be safe
|
||
target.Json.Append('"'); | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this will inline as it's quite big
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure it does inline it. It is only used in two places within the method above. Once for the none-dotnetcore-part and once for the dotnet-core part. So its only used once per compile target.
I saw an increment of execution duration without that attribute during the Benchmark-Runs - that's why I added it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool
Jint/Native/Json/JsonSerializer.cs
Outdated
@@ -547,22 +590,19 @@ public static PropertyEnumeration FromObjectInstance(ObjectInstance instance) | |||
|
|||
private static void RemoveUnserializableProperties(ObjectInstance instance, List<JsValue> keys) | |||
{ | |||
for (var i = 0; i < keys.Count; i++) | |||
for (var i = keys.Count - 1; i >= 0; i--) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I even had to open a question about this as engines seem to differ: tc39/test262#3811
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh damn, totally forgot that you can do that in JS. Damnit! Will undo the order change. It didn't contribute anything for the perf test anyway because the tests don't include any undefined
- properties or non-plain objects.
Thank you! |
You're welcome 😀 |
Summary
Now with .NET Core added as a build target, a small performance improvement can be made for JSON serialization.
This change reduces the time needed for serialization. Depending on the content to serialize, the change leads to a 10-20% time reduction. For some JSON files, it reduced the time to serialize even more, for a few of them, the improvement is almost negatable or even a little bit slower (see
citm_catalog
which takes 4% longer). The improvement works best for all ASCII-only strings (which are probably 99% of all keys for JSON objects) and for long string values (seedoj-blog.json
result for that).Changes
I've changed the signature of the internal method
NumberPrototype.NumberToString
. It doesn't return a string anymore and instead will always fill the provided StringBuilder. That change allowed me to remove theNumberBuffer
string builder during serialization which means that theJsonSerializer
will now only allocate one StringBuilder per execution.This didn't improve the performance a lot, but reduced the allocations quite significantly if a lot of floating point numbers needed to get serialized - which happens in the
canada.json
example.Stuff tried but not included
I've also tried to remove all the checks for "indentation is active -> yes/no" during serialization. Basically I've doubled the implementation - one for the indentation active case and one without any indentation and did only one check at the very start. That one improves the performance, but not significantly enough (~5%) to accept the downside of having to maintain basically two almost identical implementations in my opinion.
JSON files
The same I've used previously:
Results
To reduce the torture of the previous huge tables, I tried to make it more readable by reducing the amount of columns and group the results - first all old serialize results and then all new serializer results.
As before, the important columns are
Ratio
andAlloc Ratio
.