Remove extra allocations in PSMemberInfoInternalCollection<T> #7435

iSazonov · 2018-08-02T14:09:13Z

PR Summary

Related #7112.
PSMemberInfoInternalCollection uses internally OrderedDictionary class.
The OrderedDictionary class in .Net Core 2.1 allocates internally ArrayList and Hashtable after any first using any property. Even if we check that OrderedDictionary is empty (Count==0) or use indexer it causes allocations.

The commit postpones the internall allocations until this is really necessary.

Taking into account that PSMemberInfoInternalCollection is used in each PSObject and is often empty, this change dramatically reduces memory allocations.

PerfView results

Startup test

before - GC Stats Total Allocs : 13.274 MB
after - GC Stats Total Allocs : 10.082 MB

Script test (Import-Csv)
Before fix:

GC Stats Total Allocs : 4,073.747 MB

After the fix:

GC Stats Total Allocs : 1,186.017 MB

Test script:

cd c:\tmp\
function perf_test($source, $result) {
    Write-Host "Measuring Import-Csv performance over time..."

    "index,duration_ms,bytes_consumed" | Out-File $result
    for ($i=0; $i -le 400; $i++) {

            $millis = (Measure-Command { Import-Csv $source }).TotalMilliseconds
            # Uncomment this if you want analize results in Excel
            $memory = [System.GC]::GetTotalMemory($false)
	    $i.ToString() + "," + $millis.ToString() + "," + $memory | Out-File $result -Append
    }
    Write-Host "Done"
}

$fields = 0..19 | ForEach-Object { "random_numbers$_" }
($fields -join ",") | Out-File .\source2.csv
Get-Random -SetSeed 1 | Out-Null
for ($i=0; $i -le 500; $i++) {
    $values = 0..19 | ForEach-Object { (Get-Random).ToString() }
    ($values -join ",") | Out-File .\source2.csv -Append
}

perf_test .\source2.csv .\results.csv

PR Checklist

PSMemberInfoInternalCollection<T> uses internally OrderedDictionary class. The OrderedDictionary class in .Net Core 2.1 allocates internally ArrayList and Hashtable after any first using any property. Even if we check that OrderedDictionary is empty (Count==0) or use indexer it causes allocations. The commit postpones the internall allocations until this is really necessary. Taking into account that PSMemberInfoInternalCollection<T> is used in each PSObject and is often empty, this change dramatically reduces memory allocations.

iSazonov · 2018-08-02T14:16:48Z

@SteveL-MSFT If the PR is ok you could consider it for Windows PowerShell 5.1 too.

@daxian-dbw Sorry for ping - the PR can be important.

@markekraus Could you please check json cmdlets with the change if you have a time. I hope we get a win there too.

@vancem Thanks for your great analysis - it helped a lot!

SteveL-MSFT

LGTM. At this time we're trying not to make changes to 5.1 as we're trying to make 6 a viable replacement for 5.1 and spending time on that instead.

daxian-dbw · 2018-08-02T17:19:05Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                lock (Members)
                {
-                    return _members[name] as T;
+                    return Members[name] as T;


nitpick: since we already know _member != null when reaching here, maybe we can just continue to use _member for this lock (...) block. It's a little bit more consistent when reading the code.

daxian-dbw · 2018-08-02T18:03:02Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                if (_members == null)
+                {
+                    _members = new OrderedDictionary(StringComparer.OrdinalIgnoreCase);
+                }


There could be a race condition here.
In an exaggerated way, thread A checked _members == null and entered the if block, then A got suspended, and thread B came in and used Members (so _member is initialized). When A got resumed, it would re-create the OrderedDictionary object, and thus any members B added previously will be gone.

I suggest the following code:

if (_members == null) { Interlocked.CompareExchange(ref _members, new OrderedDictionary(StringComparer.OrdinalIgnoreCase), null); } return _members;

Great catch!

Fixed.

daxian-dbw · 2018-08-02T18:21:02Z

src/System.Management.Automation/engine/MshMemberInfo.cs

            }

-            lock (_members)
+            lock (Members)


Shall we also check for _members == null for the Remove action as a shortcut path?

I think we can add the check in public methods. Moreover, this code can be used in the most unpredictable scenarios

Fixed.

daxian-dbw · 2018-08-02T18:23:04Z

src/System.Management.Automation/engine/MshMemberInfo.cs

        {
            PSMemberInfoInternalCollection<T> returnValue = new PSMemberInfoInternalCollection<T>();
-            lock (_members)
+            lock (Members)


Same here, shall we also check for _members == null for a shortcut path?

daxian-dbw · 2018-08-02T18:23:55Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                lock (Members)
                {
-                    return _members.Count;
+                    return Members.Count;


Same nitpick here, how about just use _members for the lock(...) block?

daxian-dbw · 2018-08-02T18:24:08Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                lock (Members)
                {
-                    return _members.Count - _countHidden;
+                    return Members.Count - _countHidden;


Same nitpick here.

daxian-dbw · 2018-08-02T18:24:26Z

src/System.Management.Automation/engine/MshMemberInfo.cs

+                lock (Members)
                {
-                    return _members[index] as T;
+                    return Members[index] as T;


Same nitpick here.

daxian-dbw · 2018-08-02T18:26:17Z

src/System.Management.Automation/engine/MshMemberInfo.cs

        public override IEnumerator<T> GetEnumerator()
        {
-            lock (_members)
+            lock (Members)


if we want to go a bit far, maybe also check _members == null here and if so return Array.Empty<T>.GetEnumerator()?
Just an idea.

Good idea for the public method.

Fixed as return Enumerable.Empty<T>().GetEnumerator();

iSazonov · 2018-08-03T11:44:51Z

@daxian-dbw I reverted lock (_members) to lock (Members) because the lock failed on null in Add(). I believe that it makes no sense to add the null check in the method so using lock (Members) everywhere looks good.

iSazonov · 2018-08-03T12:29:27Z

I measure pwsh.exe startup and update PR description:

Startup test

before - GC Stats Total Allocs : 13.274 MB
after - GC Stats Total Allocs : 10.082 MB

markekraus · 2018-08-03T13:46:33Z

@iSazonov The issue with JSON is one of speed more than memory allocation. I don't have he tooling to test the memory performance. but the code I use to test the JSON performance is this:

$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

5.1: 375.3416ms
6.0.2: 2055.1092ms
6.1.0-preview.4: 1252.1725ms
6.1.0-preview.10466: 1130.4745ms (your most recent passing build for this PR)

so it is marginally better than 6.1.0-preview.4, better than 6.0.2, but still nowhere near the performance of 5.1. ☹️

iSazonov · 2018-08-03T14:24:18Z

@markekraus Many thanks for your repo script! I'll try to look it deeper next week.

Update: I did fast look - after #7413 and the PR I think we get a win in ConvertFrom-Json too.

daxian-dbw · 2018-08-03T17:04:27Z

@iSazonov You cannot change all lock(Members) to lock(_members). You can change those that _members == null has already been checked. For Add, there is no check for _members != null, so changing to lock (_members) would fail for sure.

powercode · 2018-08-03T17:13:14Z

@markekraus
There is a very expensive check for duplicate members in JsonObject.PopulateFromJDictionary.

By replacing that check, and by keeping a hashset of preValidated member, I can speed it up by a factor of 3 or so.

iSazonov · 2018-08-03T17:33:39Z

src/System.Management.Automation/engine/MshMemberInfo.cs

            }

-            lock (Members)
+            lock (_members)


@daxian-dbw Please clarify - is it ok to lock on different objects in different methods? What if different threads call Add() and Remove() in the same time?

All locks are essentially on the same object. When Add and Remove are called at the same time, there are two possibilities:

The check of _members == null in Remove happens before the initialization of _member triggered by Add;

The check of _members == null in Remove happens after the initialization of _member triggered by Add.

In the first case, Remove will just return, and Add will lock on the newly initialized object.
In the second case, Remove will try to lock on _members (once it's not null, it will never be set back to null), which points to the same object as Member, so Remove and Add will compete locking on the same object.

iSazonov · 2018-08-03T17:38:06Z

@powercode Using preValidated looks as internal only hack. It will be nice to find more generic solution that we can expose to public.

powercode · 2018-08-03T17:41:32Z

@iSazonov I agree about preValidated.

It isn't obvious how though. It would require some sort of scope, where I say now I'm gonna add a bunch of similar objects - do validation only on the first one.

powercode

Would it be better to get a local reference to the Members instead of calling the property repeatedly?

            var members = Members;
            lock (members)
            {
                if (members[member.Name] is T existingMember)
                {
                    Replace(existingMember, member);
                }
                else
                {
                    members[member.Name] = member;
                    if (member.IsHidden)
                    {
                        _countHidden++;
                    }
                }
            }

daxian-dbw · 2018-08-03T18:05:02Z

One test failed in AppVeyor, but works fine on my dev box. Will address @powercode's comment and trigger the CIs again.

[00:15:18]     [-] -WindowStyle Maximized should work on Windows 
[00:15:18]       Expected exactly {Maximized}, but got {Hidden}.
[00:15:18]       636:         $showCmd | Should -BeExactly $WindowStyle
[00:15:18]       at Invoke-Assertion, C:\projects\powershell\src\powershell-win-core\bin\Release\netcoreapp2.1\win7-x64\publish\Modules\Pester\4.2.0\Functions\Assertions\Should.ps1: line 206
[00:15:18]       at <ScriptBlock>, C:\projects\powershell\test\powershell\Host\ConsoleHost.Tests.ps1: line 636
[00:15:18] Processing -WindowStyle 'invalid' failed: Cannot convert value "invalid" to type "System.Diagnostics.ProcessWindowStyle". Error: "Unable to match the identifier name invalid to a valid enumerator name. Specify one of the following enumerator names and try again:
[00:15:18] Normal, Hidden, Minimized, Maximized".

iSazonov · 2018-08-07T03:33:35Z

Thanks all for help and useful comments!

iSazonov · 2018-08-31T13:38:45Z

OrderedDictionary was fixed in dotnet/corefx#32001 - some properies now don't allocate if no need.

iSazonov requested review from BrucePay, SteveL-MSFT and daxian-dbw August 2, 2018 14:09

SteveL-MSFT requested a review from adityapatwardhan August 2, 2018 15:51

SteveL-MSFT approved these changes Aug 2, 2018

View reviewed changes

daxian-dbw requested changes Aug 2, 2018

View reviewed changes

iSazonov added 6 commits August 3, 2018 08:08

Use _member in lock

4d900c6

Return empty enumerator

15900b8

Use Interlocked.CompareExchange in initialization code

8ccf586

Add short return path in Remove() method

6ad4a71

Fix Count and VisiableCount

d94a03f

Fix indexer

feecfd6

daxian-dbw self-assigned this Aug 3, 2018

Revert commit "Use _member in lock"

fb9efec

[Feature] Minor updates

9bd2e07

iSazonov commented Aug 3, 2018

View reviewed changes

powercode reviewed Aug 3, 2018

View reviewed changes

[Feature] Address comment

57dd2c4

daxian-dbw approved these changes Aug 3, 2018

View reviewed changes

daxian-dbw merged commit 097e144 into PowerShell:master Aug 6, 2018

iSazonov deleted the remove-extra-allocations-in-psmemberinfointernalcollection branch August 7, 2018 03:33

iSazonov mentioned this pull request Aug 31, 2018

Interfaces should be included in TypeNames for PSObjects #7649

Closed

iSazonov mentioned this pull request Jan 31, 2020

It is impossible to determine whether the OrderedDictionary is empty without extra allocations dotnet/runtime#27242

Closed

Remove extra allocations in PSMemberInfoInternalCollection<T> #7435

Remove extra allocations in PSMemberInfoInternalCollection<T> #7435

Uh oh!

Conversation

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

Uh oh!

PR Summary

PerfView results

PR Checklist

Uh oh!

iSazonov commented Aug 2, 2018

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Aug 2, 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!

iSazonov Aug 3, 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!

daxian-dbw Aug 2, 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!

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

Uh oh!

Uh oh!

iSazonov commented Aug 3, 2018

Uh oh!

markekraus commented Aug 3, 2018

Uh oh!

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

Uh oh!

Uh oh!

daxian-dbw commented Aug 3, 2018

Uh oh!

powercode commented Aug 3, 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!

daxian-dbw Aug 3, 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 commented Aug 3, 2018

Uh oh!

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

Uh oh!

Uh oh!

powercode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

iSazonov commented Aug 2, 2018 •

edited

Loading

daxian-dbw Aug 2, 2018 •

edited

Loading

iSazonov Aug 3, 2018 •

edited

Loading

daxian-dbw Aug 2, 2018 •

edited

Loading

iSazonov commented Aug 3, 2018 •

edited

Loading

iSazonov commented Aug 3, 2018 •

edited

Loading

powercode commented Aug 3, 2018 •

edited

Loading

daxian-dbw Aug 3, 2018 •

edited

Loading

powercode commented Aug 3, 2018 •

edited

Loading

powercode left a comment •

edited

Loading