Improve performance on json to psobject conversion #7482

powercode · 2018-08-08T10:36:57Z

PR Summary

Changes to the JsonObject class to make conversions from json to PSObjects faster.

Three main changes:
Convert from JArray directly to object instead of creating a list and do list.ToArray().
Do not use PSObject to check for existing members, since that is a very slow and allocation heavy (sic!) code path.
PreAllocation the members in PSObject by using the newly added constructor accepting an initial member count.

This amounts to a speed-up in the order of 7x.

Cmdlets that benefit are Convertfrom-Json and Invoke-RestMethod.

PR Checklist

Using pattern matching to simplify code. Removing some redundant initializations. nameof(input) instead of "input" in exception. Simplification of assignment Removing code redundancies

markekraus · 2018-08-08T13:43:15Z

$json = @'
{
    "id": "string",
    "data": [
        {                       
        "accountId": 0,
        "productId": 0,
        "resourceLocationId": 0,
        "consumedServiceId": 0,
        "departmentId": 0,
        "accountOwnerEmail": "string",
        "accountName": "string",
        "serviceAdministratorId": "string",
        "subscriptionId": 0,
        "subscriptionGuid": "string",
        "subscriptionName": "string",
        "date": "2017-04-27T23:01:43.799Z",
        "product": "string",
        "meterId": "string",
        "meterCategory": "string",
        "meterSubCategory": "string",
        "meterRegion": "string",
        "meterName": "string",
        "consumedQuantity": 0,
        "resourceRate": 0,
        "Cost": 0,
        "resourceLocation": "string",
        "consumedService": "string",
        "instanceId": "string",
        "serviceInfo1": "string",
        "serviceInfo2": "string",
        "additionalInfo": "string",
        "tags": "string",
        "storeServiceIdentifier": "string",
        "departmentName": "string",
        "costCenter": "string",
        "unitOfMeasure": "string",
        "resourceGroup": "string"
        }
    ],
    "nextLink": "string"
}
'@
$json = 1..1000 | % { $json }
$json = '[' + ($json -join ',') + ']'
Measure-Command {$Json | ConvertFrom-Json } | % TotalMilliseconds

Runs in 497.8867 miliseconds on the most recent build in this PR vs 1104.9008 ms in preview.4 and 318.1642ms in 5.1. Much better!! Thanks @powercode

markekraus

LGTM

powercode · 2018-08-08T13:50:16Z

Did you test that with a crossgen build?

I know numbers! I have a lot of numbers. A lot of people don't know that, but I have the best numbers!

Same code but with 20000 iterations - I get 27091 ms on 6.0.3 and 3635 ms on the PR.

Curious about the difference.
My crossgen:ed build has 177 ms for 1000 iterations vs 1312 ms on 6.0.3.

Start-PSBuild -Crossgen -Configuration release

daxian-dbw · 2018-08-08T17:28:25Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
-                    string errorMsg = string.Format(CultureInfo.InvariantCulture,
-                                                    WebCmdletStrings.DuplicateKeysInJsonString, entry.Key);
+                    var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key);


minor: CurrentCulture should be used when constructing error message. A flaw in the original code.

daxian-dbw · 2018-08-08T17:53:40Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

-                    }
+                    obj = returnHashTable ?
+                              PopulateHashTableFromJDictionary(dictionary, out error) :
+                              PopulateFromJDictionary(dictionary, memberTracker, out error);


When reaching here, we already know the size of the JObject, so maybe we can replace memberTracker with new DuplicateMemberSet(dictionary.Count)? (just like what you did in other places)
Then DefaultMemberSetCapacity won't be needed.

daxian-dbw · 2018-08-08T17:56:00Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

            foreach (var entry in entries)
            {
                if (string.IsNullOrEmpty(entry.Key))
                {
-                    string errorMsg = string.Format(CultureInfo.InvariantCulture,
-                                                    WebCmdletStrings.EmptyKeyInJsonString);
+                    var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.EmptyKeyInJsonString);


minor: CurrentCulture should be used when constructing error strings. It's a flaw in the original code.

daxian-dbw · 2018-08-08T17:56:41Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
-                    string errorMsg = string.Format(CultureInfo.InvariantCulture,
-                                                    WebCmdletStrings.KeysWithDifferentCasingInJsonString, property.Name, entry.Key);
+                    var errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.KeysWithDifferentCasingInJsonString, propertyName, entry.Key);


minor: CurrentCulture should be used when constructing error message.

daxian-dbw · 2018-08-08T17:57:07Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

@@ -255,8 +246,7 @@ private static Hashtable PopulateHashTableFromJDictionary(JObject entries, out E
                // does not throw when encountering duplicates and just uses the last entry.
                if (result.ContainsKey(entry.Key))
                {
-                    string errorMsg = string.Format(CultureInfo.InvariantCulture,
-                                                    WebCmdletStrings.DuplicateKeysInJsonString, entry.Key);
+                    string errorMsg = string.Format(CultureInfo.InvariantCulture, WebCmdletStrings.DuplicateKeysInJsonString, entry.Key);


minor: same here

daxian-dbw · 2018-08-08T17:58:42Z

Good perf work @powercode! The changes look good to me in general. Left a few minor comments.

daxian-dbw

Ship it! 🎉

powercode · 2018-08-08T18:36:03Z

I looked a bit at the difference in perf to Windows PowerShell.

Newtonsoft.Json.JsonConvert.DeserializeObject is slower than System.Web.Script.Serialization.JavaScriptObjectDeserializer. Looking at the code to see if I can figure out what the root cause is.

dantraMSFT

Other than the minor suggestion, LGTM.

dantraMSFT · 2018-08-08T19:09:46Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

-                    }
+                    obj = returnHashTable ?
+                              PopulateHashTableFromJDictionary(dictionary, out error) :
+                              PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error);
                }
                else


Minor suggestion: the way the if/else block is structured, it implies a path that is not covered. I suggest using if/else if/else and use the else block to provide an explicit comment.

if (object isJObject dictionary)
{}
else if (obj is JArray list)
{}
else
{ # nothing to do; return the obj as is.
}

I'm moving the return into the try statement so I get

if (object is JObject dictionary) {...} else if (obj is JArray list) {///} // if obj is a anything but JArray or JObject, we just return as is return obj;

Then it's clear anyway that we return obj as is for the other cases.

Or how about

switch (obj) { case JObject dictionary: return returnHashTable ? PopulateHashTableFromJDictionary(dictionary, out error) : PopulateFromJDictionary(dictionary, new DuplicateMemberSet(dictionary.Count), out error); case JArray list: return returnHashTable ? PopulateHashTableFromJArray(list, out error) : PopulateFromJArray(list, out error); default: return obj; }

?

That makes the alternative really clear.

iSazonov · 2018-08-09T07:48:29Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

        }

        /// <summary>
-        /// Convert a Json string back to an object of type PSObject or Hashtable depending on parameter <paramref name="returnHashTable"/>.
+        /// Convert a Json string back to an object of type PSObject or HashTable depending on parameter <paramref name="returnHashTable"/>.


The class name is Hashtable not HashTable.

iSazonov · 2018-08-09T07:51:20Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

            foreach (var entry in entries)
            {
                if (string.IsNullOrEmpty(entry.Key))
                {
-                    string errorMsg = string.Format(CultureInfo.InvariantCulture,
-                                                    WebCmdletStrings.EmptyKeyInJsonString);
+                    var errorMsg = string.Format(CultureInfo.CurrentCulture, WebCmdletStrings.EmptyKeyInJsonString);


We should use our help method StringUtil.Format. Below too.

iSazonov · 2018-08-09T07:53:33Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

        {
            error = null;
-            PSObject result = new PSObject();
+            var result = new PSObject(entries.Count);
            foreach (var entry in entries)


It would be nice to replace the var with explicit type. Please.

The type is clearly stated by the ctor. Why repeat it?

Our coding guidelines has

var example = obj as Example;

as an example. I don't see how this is any different. There can be no confusion as to what type result has.

In general, I think it would be better to consider types as something the compiler uses to ensure we don't use something in an unintended way.

Personally, I find all the type annotations reducing readability, since they get in the way of the essence.

Again, as background, I really recommend Herb Sutters https://herbsutter.com/2013/08/12/gotw-94-solution-aaa-style-almost-always-auto/

Also, when looking at high quality, modern, C# code bases, use of var is very common, and in my eyes non-controversial.

https://github.com/dotnet/roslyn/blob/acd31862be412fcb4f7e0bc7a7db63ea8228a327/src/Compilers/Shared/DesktopBuildClient.cs#L39

https://github.com/dotnet/roslyn/blob/master/src/Compilers/Shared/BuildServerConnection.cs

Do we both say about foreach (var entry in entries) ? Seems you comment var result = new PSObject(entries.Count);.

The foreach (var entry in entries) is not your code so it is my minor comment with "Please". Feel free to ignore.

Prevously we discussed this - not all code readers use power IDEs with IntelliSense and it is better for readability to use explicit type until this type is not obvious from the name of a variable or local context. Compare: foreach (var entry in intEntries) and foreach (var entry in entries) .
Above the entries variable is defined as JObject entries - at first glance it is not at all clear what type of the entry is.

I don't think that the type of entry is of interest here. It is some entity that has a Key and a Value. Should that type change, nothing in the code would need to change, as long as it has a Key and a Value.
That is captured nicely with the use of var.

I think we should only specify the type explicitly when it is important for some reason that the value is of a certain type. That way, it calls attention to it - something interesting is happening here since we explicitly specified the type. It may be that a variable is declared as a base type instead of the actual type returned by a function.

The C# coding conventions on msdn has actually captured this:

Use implicit typing for local variables when the type of the variable is obvious from the right side of the assignment, or when the precise type is not important.

Use implicit typing to determine the type of the loop variable in for and foreach loops.

The use of var and an explicit type is not just a matter of taste. They express different things. The explicit type should be used then the type matters, not as some security blanket.

var should be used when we want to express that the type is not important here, as long as it walks like a duck and quacks like a duck.

It is a semantics change. Specifying a type may for example trigger implicit type conversion operators, or throw exceptions (in the foreach case).

iSazonov · 2018-08-09T08:14:28Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
-                    JValue theValue = entry.Value as JValue;
+                    // Value
+                    JValue theValue = (JValue)entry.Value;
                    result.Properties.Add(new PSNoteProperty(entry.Key, theValue.Value));


As far as I remember, we discussed this earlier and concluded that it is safer to use as in the place.

/cc @markekraus

I'm fine with using casting when we are for sure expecting the cast-to type.
If the value does happen to be a different type, then it's a bug that needs to be fixed, and casting operator will make the bug to surface easily (while as in this case might make the bug harder to be noticed).

it's a bug that needs to be fixed

@daxian-dbw If I remember right in previous discussion we concluded that we don't want an exception here. So I ping @markekraus to confirm or reject this.

Also we could consider the change as out of the PR and revert and open a tracking issue.

@iSazonov I don't recall any reason why we need as over casting here.

iSazonov · 2018-08-09T08:22:19Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

@@ -142,10 +124,10 @@ private static PSObject PopulateFromJDictionary(JObject entries, out ErrorRecord

                // Case sensitive duplicates should normally not occur since JsonConvert.DeserializeObject
                // does not throw when encountering duplicates and just uses the last entry.
-                if (result.Properties.Any(psPropertyInfo => psPropertyInfo.Name.Equals(entry.Key, StringComparison.InvariantCulture)))
+                if (memberTracker.TryGetValue(entry.Key, out var maybePropertyName)
+                    && string.Compare(entry.Key, maybePropertyName, StringComparison.CurrentCulture) == 0)


In any case if we have a duplicate key we return "null". So we should remove the string.Compare at all.

/cc @markekraus What do you think?

We throw different errors in different cases. A duplicate key with same casing is a hard "NO", while a duplicate key with different casing is a workaround-able.

iSazonov · 2018-08-09T08:29:09Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
-                    result.Add(((JValue)element).Value);
+                    // Value
+                    result[index] = ((JValue)element).Value;


The same about as.

/cc @markekraus

iSazonov · 2018-08-09T08:31:38Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
+                    // Value
                    JValue theValue = entry.Value as JValue;


The same about as. Here we don't use cast - the pattern we approved previously.

/cc @markekraus

Yeah, this is a bit inconsistent in this file.

iSazonov · 2018-08-09T08:31:59Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

                {
-                    result.Add(((JValue)element).Value);
+                    // Value
+                    result[index] = ((JValue)element).Value;


The same about as.

/cc @markekraus

iSazonov · 2018-08-09T08:33:10Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

@@ -24,45 +19,49 @@ namespace Microsoft.PowerShell.Commands
    [SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly")]
    public static class JsonObject
    {
-        private const int maxDepthAllowed = 100;
+        private class DuplicateMemberSet : HashSet<string>


DuplicateMemberHashSet looks more readable.

iSazonov · 2018-08-09T08:34:24Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/JsonObject.cs

-        private const int maxDepthAllowed = 100;
+        private class DuplicateMemberSet : HashSet<string>
+        {
+            public DuplicateMemberSet(int capacity) : base(capacity, StringComparer.CurrentCultureIgnoreCase)


I'd expect StringComparer.InvariantCultureIgnoreCase. Why would we want to do culture sensetive compares (if we get Json string from Internet)?
This is consistent with the comment in the line 125.

/cc @markekraus

@iSazonov Regarding HashTable: StyleCop complains that the word Hashtable is spelled incorrectly, hence the change. I'm not religious about it, and can go either way. Also, the parameter is named returnHashTable. Confusing. @dantraMSFT, @markekraus Any native English speaker care to weigh in here? Maybe change all references to Hashtable casing? And maybe add hashtable as a dictionary work.

HashTable looks ugly to me :)

I think you are correct that CurrentCultureIgnoreCase is wrong. The previous code used the PSObject.Properties, and that uses OrdinalIgnoreCase, for it's member dictionary.

Not sure if we should use InvariantCultureIgnoreCase or OrdinalIgnoreCase.

I vote for OrdinalIgnoreCase.

I agree about OrdinalIgnoreCase

@powercode I think if we reference Hashtable class in comments we should use quotes like 'Hashtable' to bypass StypeCop warnings. As for variable names we should follow Camel style so returnHashTable is ok.

Using OrdinalIgnoreCase for DuplicateMemberHashSet. Renaming DuplicateMemberSet -> DuplicateMemberHashSet Renaming returnHashTable -> returnHashtable Changing help to not use the HashTable casing.

powercode · 2018-08-10T10:23:55Z

Regarding the as controversy: how about using a type switch on the JTokens?

                switch (entry.Value)
                {
                    case JArray list:
                    {
                        // Array
                        var listResult = PopulateHashTableFromJArray(list, out error);
                        if (error != null)
                        {
                            return null;
                        }

                        result.Add(entry.Key, listResult);
                        break;
                    }
                    case JObject dic:
                    {
                        // Dictionary
                        var dicResult = PopulateHashTableFromJDictionary(dic, out error);
                        if (error != null)
                        {
                            return null;
                        }

                        result.Add(entry.Key, dicResult);
                        break;
                    }
                    case JValue value:
                    {
                        result.Add(entry.Key, value.Value);
                        break;
                    }
                }

Doesn't that clearly capture what we want to express?

iSazonov · 2018-08-10T11:54:37Z

Good catch! I agree with using a type switch.

…ent. Also specifying initial capacity on hashtable.

powercode · 2018-08-10T12:04:47Z

@iSazonov I'm getting code factor errors on

switch (x)
{
    case 1:
    {
        break;
    }
    case 2:
    {
        break;
    }
}

It seems to want it like

switch (x)
{
    case 1:
    {
        break;
    }

    case 2:
    {
        break;
    }
}

That feels strange. Do you know if it is intentional or a side effect?

iSazonov

LGTM.

markekraus · 2018-08-10T13:05:34Z

@powercode I believe that spacing rule is intentional. It is easier to read code when there is some white space separating the end of one code block and the beginning of another.

iSazonov · 2018-08-10T13:39:19Z

@markekraus Thanks for review!

powercode added 3 commits August 8, 2018 12:12

Code cleanup and simplification

a6e7e1d

Using pattern matching to simplify code. Removing some redundant initializations. nameof(input) instead of "input" in exception. Simplification of assignment Removing code redundancies

Using array directly instead of list + .ToArray() to reduce allocations.

44a8bc4

Use a hashset as a faster method of tracking member duplicates.

ac4061c

powercode requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners August 8, 2018 10:36

CodeFactor fixes

f565dd8

markekraus approved these changes Aug 8, 2018

View reviewed changes

daxian-dbw reviewed Aug 8, 2018

View reviewed changes

daxian-dbw self-assigned this Aug 8, 2018

Addressing dongbo's comments.

58ef7cc

daxian-dbw approved these changes Aug 8, 2018

View reviewed changes

dantraMSFT reviewed Aug 8, 2018

View reviewed changes

addressing dantraMSFT comments - part 2

64f33ea

dantraMSFT approved these changes Aug 8, 2018

View reviewed changes

iSazonov changed the title ~~Perf: Improve performance on json to psobject conversion~~ Improve performance on json to psobject conversion Aug 9, 2018

iSazonov reviewed Aug 9, 2018

View reviewed changes

iSazonov requested a review from markekraus August 9, 2018 09:11

Addressing review comments

bb15645

Using OrdinalIgnoreCase for DuplicateMemberHashSet. Renaming DuplicateMemberSet -> DuplicateMemberHashSet Renaming returnHashTable -> returnHashtable Changing help to not use the HashTable casing.

powercode added 2 commits August 10, 2018 13:57

Type switch on JArray, JObject and JValue to more clearly specify int…

d345979

…ent. Also specifying initial capacity on hashtable.

Spacing around switch.

9a5dea0

iSazonov approved these changes Aug 10, 2018

View reviewed changes

markekraus approved these changes Aug 10, 2018

View reviewed changes

daxian-dbw merged commit 1243891 into PowerShell:master Aug 10, 2018

TravisEz13 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Mar 29, 2019

Improve performance on json to psobject conversion #7482

Improve performance on json to psobject conversion #7482

Uh oh!

Conversation

powercode commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

markekraus commented Aug 8, 2018

Uh oh!

markekraus left a comment

Choose a reason for hiding this comment

Uh oh!

powercode commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Aug 8, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

powercode commented Aug 8, 2018

Uh oh!

dantraMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

powercode commented Aug 8, 2018 •

edited

Loading

powercode commented Aug 8, 2018 •

edited

Loading

powercode Aug 8, 2018 •

edited

Loading

powercode Aug 9, 2018 •

edited

Loading

iSazonov Aug 10, 2018 •

edited

Loading

powercode Aug 10, 2018 •

edited

Loading

powercode Aug 9, 2018 •

edited

Loading

powercode commented Aug 10, 2018 •

edited

Loading

powercode commented Aug 10, 2018 •

edited

Loading