Fixing CopyFrom method in OdataMessageReader and OdataMessageWriter to include Arraypool #1595

Sreejithpin · 2019-11-13T20:56:46Z

Issues

There was an issue that OData.Core ICharArrayPool is not hooked up properly, which was causing lot of Large Object Heap allocations taking up lot of memory

Description

Added Arrayppol copying in ODataMessage reader and writer settings
Added test cases for checking the same
Added generic test method to compare 2 objects (deep copy)

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

N/A

KanishManuja-MS · 2019-11-19T17:20:55Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataMessageReaderSettingsTests.cs

@@ -132,7 +132,7 @@ public void ODataMessageReaderCopyConstructorTest()
            // Compare original and settings created from copy constructor after setting some values
            settings.BaseUri = new Uri("http://www.odata.org");
            settings.EnableCharactersCheck = true;
-            copyOfSettings = settings.Clone();
+            copyOfSettings = settings.Clone(); 


Unintended change.

KanishManuja-MS · 2019-11-19T17:24:37Z

test/FunctionalTests/Microsoft.OData.Core.Tests/TestCharArrayPool.cs

+
+        }
+    }
+


Remove this line.

KanishManuja-MS · 2019-11-19T17:26:06Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+                            if (element1 == null || element2 == null)
+                            {
+                                diff.Add(string.Format("Value of items in Collection {0} does not match.", prop.Name));
+


extra line.

KanishManuja-MS · 2019-11-19T17:26:27Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+                            {
+                                EvaluateDifferences(element1, element2, diff);
+                            }
+


nit: extra line.

KanishManuja-MS · 2019-11-19T17:30:50Z

test/EndToEndTests/Tests/Client/Build.Desktop/Microsoft.Test.OData.Tests.Client.NetCore.csproj

  <ItemGroup>
    <Folder Include="References\" />
  </ItemGroup>
-
-</Project>


Does this have any change? If it is a change that you did not intend then please revert this.

KanishManuja-MS · 2019-11-19T17:31:12Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataMessageWriterSettingsTests.cs

+
+            var differences = ValidationHelper.GetDifferences<ODataMessageWriterSettings>(this.settings, newSetting);
+            Assert.True(differences.Count == 0, String.Join(",", differences));
+


nit: extra line.

KanishManuja-MS · 2019-11-19T17:31:33Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataMessageWriterSettingsTests.cs

@@ -357,7 +400,7 @@ public void PropertyGettersAndSettersTest()
                    MaxNestingDepth = maxNestingDepth,
                },
                Version = version,
-                LibraryCompatibility = library,
+                LibraryCompatibility = library,                


revert whitespace.

xuzhg · 2019-11-19T17:35:44Z

test/FunctionalTests/Microsoft.OData.Core.Tests/TestCharArrayPool.cs

+
+namespace Microsoft.OData.Tests
+{
+    public class TestCharArrayPool : ICharArrayPool


I remember I had a default implementation for ICharArrayPool. Don't I?

kept the Testchararraypool as such to be reused later as the the testarraypool implementation was inside another class file.

xuzhg · 2019-11-19T17:36:34Z

...ctionalTests/Microsoft.OData.Core.Tests/Build.NetFramework/Microsoft.OData.Core.Tests.csproj

@@ -281,6 +281,9 @@
    <Compile Include="..\ScenarioTests\UriParser\TopAndSkipFunctionalTests.cs" />
    <Compile Include="..\ScenarioTests\UriParser\VerificationHelpers.cs" />
    <Compile Include="..\ScenarioTests\Writer\JsonLight\WriteFeedWithoutNavigationSourceTests.cs" />
+    <Compile Include="..\TestCharArrayPool.cs">
+      <Link>TestCharArrayPool.cs</Link>


Shall we add the for the Compile element?

xuzhg · 2019-11-19T17:36:37Z

...ctionalTests/Microsoft.OData.Core.Tests/Build.NetFramework/Microsoft.OData.Core.Tests.csproj

@@ -450,6 +453,9 @@
    <Compile Include="..\UriParser\Visitors\IsCollectionTranslatorTests.cs" />
    <Compile Include="..\UriParser\Visitors\QueryNodeVisitorTests.cs" />
    <Compile Include="..\UriParser\Visitors\SyntacticTreeVisitorTests.cs" />
+    <Compile Include="..\ValidationHelper.cs">
+      <Link>ValidationHelper.cs</Link>


Shall we add the for the Compile element?

xuzhg · 2019-11-19T17:36:53Z

test/EndToEndTests/Tests/Client/Build.Desktop/Microsoft.Test.OData.Tests.Client.NetCore.csproj

@@ -1,147 +1,147 @@
-<Project Sdk="Microsoft.NET.Sdk">


why change this file?

Fixes based on Review Comments

This reverts commit 1345914, reversing changes made to 9a786ed.

…/odata.net into UpdateCopyFromMethod

Reverting Microsoft.Test.OData.Tests.Client.NetCore

reverted stylecop buildnumber as its already committed

xuzhg · 2019-11-19T21:01:52Z

test/FunctionalTests/Microsoft.OData.Core.Tests/TestCharArrayPool.cs

@@ -0,0 +1,27 @@
+using Microsoft.OData.Buffers;
+using System;
+


xuzhg · 2019-11-19T21:01:58Z

test/FunctionalTests/Microsoft.OData.Core.Tests/TestCharArrayPool.cs

@@ -0,0 +1,27 @@
+using Microsoft.OData.Buffers;
+using System;


xuzhg · 2019-11-19T21:02:06Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;


xuzhg · 2019-11-19T21:02:33Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+
+namespace Microsoft.OData.Tests
+{
+    public class ValidationHelper


class ValidationHelper [](start = 11, length = 22)

make it as "static class"?

xuzhg · 2019-11-19T21:02:54Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+        /// </summary>
+        /// <typeparam name="T"></typeparam>
+        /// <param name="obj"></param>
+        /// <param name="copy"></param>


If you add the comments, finish the comments

xuzhg · 2019-11-19T21:03:23Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+        /// <typeparam name="T"></typeparam>
+        /// <param name="obj"></param>
+        /// <param name="copy"></param>
+        /// <param name="diff"></param>


xuzhg · 2019-11-19T21:03:50Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+        private static void EvaluateDifferences<T>(T obj, T copy, List<string> diff)
+        {
+
+            foreach (var prop in obj.GetType().GetProperties().Where(x => x.CanRead))


verify the input and remove unused the line

xuzhg · 2019-11-19T21:10:03Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+                            else
+                            {
+                                EvaluateDifferences(element1, element2, diff);
+                            }


It seems you can replace all by calling EvaluateDifferences again?

The text for this contains collection, eg: "Values of Collection {0} does not match.". So to keep that text and any further modificatioins needed for collection did like this. Pls let me know if thats fine. Let me try anyway to make it more generic at a single place

xuzhg · 2019-11-19T21:11:19Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataMessageReaderSettingsTests.cs

@@ -205,6 +205,9 @@ private void CompareMessageReaderSettings(ODataMessageReaderSettings expected, O
            Assert.True(expected.MessageQuotas.MaxOperationsPerChangeset == actual.MessageQuotas.MaxOperationsPerChangeset, "MaxOperationsPerChangeset does not match");
            Assert.True(expected.MessageQuotas.MaxNestingDepth == actual.MessageQuotas.MaxNestingDepth, "MaxNestingDepth does not match");
            Assert.True(expected.MessageQuotas.MaxReceivedMessageSize == actual.MessageQuotas.MaxReceivedMessageSize, "MaxMessageSize does not match");
+
+            var differences = ValidationHelper.GetDifferences<ODataMessageReaderSettings>(expected, actual);
+            Assert.True(differences.Count == 0, String.Join(",", differences));      


If you add the "GetDifferences" function call, how about the codes from Line 191 to Line 207?

Didn't want to change the existing code. Existing lines will function as such

That's my concern. It seems we verify the same twice.

In reply to: 348177998 [](ancestors = 348177998)

Added copyright, sorted usings etc

Restructured EvaluateDifferences

xuzhg · 2019-11-20T01:29:27Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ValidationHelper.cs

+        /// <param name="diff">List Of Differences</param>
+        /// <param name="objName">Optional parameter for objectname, to be used in differences string, default empty string</param>
+        /// <param name="isCollection">Optional parameter to see if its a collection, default false</param>
+        private static void EvaluateDifferences<T>(T obj, T copy, List<string> diff,string objName ="", bool isCollection =false)


,string objName ="" [](start = 83, length = 19)

add the whitespace correctly. for example:
add whitespace between the parameter, add the whitespace before and after "="?

xuzhg · 2019-11-20T01:31:06Z

test/FunctionalTests/Microsoft.OData.Core.Tests/ODataMessageReaderSettingsTests.cs

-            Assert.True(expected.MessageQuotas.MaxReceivedMessageSize == actual.MessageQuotas.MaxReceivedMessageSize, "MaxMessageSize does not match");
+
+            var differences = ValidationHelper.GetDifferences<ODataMessageReaderSettings>(expected, actual);
+            Assert.True(differences.Count == 0, String.Join(",", differences));      


[](start = 79, length = 6)

remove the unused whitespace

xuzhg

…o include Arraypool (OData#1595) * Changes and test * Fixing the build error * Fixes based on Review Comments Fixes based on Review Comments * Update Microsoft.Test.OData.Tests.Client.NetCore.csproj * Revert "Merge branch 'master' into UpdateCopyFromMethod" This reverts commit 1345914, reversing changes made to 9a786ed. * Update Microsoft.Test.OData.Tests.Client.NetCore.csproj Reverting Microsoft.Test.OData.Tests.Client.NetCore * reverted stylecop buildnumber reverted stylecop buildnumber as its already committed * Fixed as per review comments Added copyright, sorted usings etc * Restructured EvaluateDifferences Restructured EvaluateDifferences * Update ValidationHelper.cs

…o include Arraypool (#1595) * Changes and test * Fixing the build error * Fixes based on Review Comments Fixes based on Review Comments * Update Microsoft.Test.OData.Tests.Client.NetCore.csproj * Revert "Merge branch 'master' into UpdateCopyFromMethod" This reverts commit 1345914, reversing changes made to 9a786ed. * Update Microsoft.Test.OData.Tests.Client.NetCore.csproj Reverting Microsoft.Test.OData.Tests.Client.NetCore * reverted stylecop buildnumber reverted stylecop buildnumber as its already committed * Fixed as per review comments Added copyright, sorted usings etc * Restructured EvaluateDifferences Restructured EvaluateDifferences * Update ValidationHelper.cs

Changes and test

1700fe9

Sreejithpin requested review from xuzhg and KanishManuja-MS November 13, 2019 20:56

Fixing the build error

84a1478

Sreejithpin changed the title ~~Changes and test~~ Fixing CopyFrom method in OdataMessageReader and OdataMessageWriter to include Arraypool Nov 13, 2019

KanishManuja-MS reviewed Nov 19, 2019

View reviewed changes

test/FunctionalTests/Microsoft.OData.Core.Tests/TestCharArrayPool.cs Outdated

}

}

Copy link

Contributor

KanishManuja-MS Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line.

KanishManuja-MS reviewed Nov 19, 2019

View reviewed changes

xuzhg reviewed Nov 19, 2019

View reviewed changes

Sreejith Pazhampilly and others added 7 commits November 19, 2019 11:34

Fixes based on Review Comments

9a786ed

Fixes based on Review Comments

Update Microsoft.Test.OData.Tests.Client.NetCore.csproj

2f5ba1b

Merge branch 'master' into UpdateCopyFromMethod

1345914

Revert "Merge branch 'master' into UpdateCopyFromMethod"

00685d0

This reverts commit 1345914, reversing changes made to 9a786ed.

Merge branch 'UpdateCopyFromMethod' of https://github.com/Sreejithpin…

6f277b7

…/odata.net into UpdateCopyFromMethod

Update Microsoft.Test.OData.Tests.Client.NetCore.csproj

0cddbf5

Reverting Microsoft.Test.OData.Tests.Client.NetCore

reverted stylecop buildnumber

5fa540e

reverted stylecop buildnumber as its already committed

xuzhg reviewed Nov 19, 2019

View reviewed changes

Sreejith Pazhampilly added 2 commits November 19, 2019 13:28

Fixed as per review comments

bbfbdaa

Added copyright, sorted usings etc

Restructured EvaluateDifferences

bd57e09

Restructured EvaluateDifferences

xuzhg reviewed Nov 20, 2019

View reviewed changes

xuzhg approved these changes Nov 20, 2019

View reviewed changes

Update ValidationHelper.cs

a2471c5

KanishManuja-MS approved these changes Nov 20, 2019

View reviewed changes

KanishManuja-MS merged commit c2b53e8 into OData:master Nov 20, 2019


		var differences = ValidationHelper.GetDifferences<ODataMessageWriterSettings>(this.settings, newSetting);
		Assert.True(differences.Count == 0, String.Join(",", differences));

Fixing CopyFrom method in OdataMessageReader and OdataMessageWriter to include Arraypool #1595

Fixing CopyFrom method in OdataMessageReader and OdataMessageWriter to include Arraypool #1595

Conversation

Sreejithpin commented Nov 13, 2019

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sreejithpin Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhg left a comment

Choose a reason for hiding this comment

Sreejithpin Nov 19, 2019 •

edited

Loading