Performance enhancements to validation and reference resolution #241

darrelmiller · 2018-04-15T21:42:15Z

The primary problem resolved in this PR is the walker, which is used for both reference resolution and validation, was walking objects multiple times when they were $ref'd. This has been resolved in the OpenApiWalker by adding a guard that does not walk the object if it has a non-null Reference and we are currently not walking through the Components section.

For the large 30MB, million line MS Graph OpenAPI, the time has gone from > 20mins, to ~ 10 secs on my Surface Pro 4.

I also added some statistics to the workbench to get an overview of the parsed file. e.g. for the Graph API file it produces:

Path Items: 6783
Operations: 10609
Parameters: 23392
Request Bodies: 3815
Responses: 10609
Links: 132
Callbacks: 0
Schemas: 336995

Also updated Workbench to allow taking input from a file.

PerthCharern · 2018-04-17T17:30:11Z

Instead of creating a property InComponents, can we change the signature of Walk for Walk(OpenApiParameter parameter) to Walk(OpenApiParameter parameter, bool isInComponents) and pass the boolean value in the caller?

It seems a bit strange that OpenApiVisitorBase needs to keep a state. #Closed

PerthCharern · 2018-04-17T17:32:03Z

src/Microsoft.OpenApi/Validations/OpenApiValidator.cs

@@ -170,7 +169,15 @@ private void Validate<T>(T item)
        private void Validate(object item, Type type)
        {
            if (item == null) return;  // Required fields should be checked by higher level objects
-            var rules = _ruleSet.Where(r => r.ElementType == type);
+


nit, per style guideline and consistency, please convert this to

if ( item == null )
{
return;
} #Closed

PerthCharern · 2018-04-17T17:33:29Z

src/Microsoft.OpenApi/Validations/ValidationRuleSet.cs

+            // We create a new instance of ValidationRuleSet per call as a safeguard
+            // against unintentional modification of the private _defaultRuleSet.
+            return new ValidationRuleSet();
+        }
        /// <summary>


nit, newline #Closed

PerthCharern · 2018-04-17T17:33:34Z

src/Microsoft.OpenApi/Validations/ValidationRuleSet.cs

@@ -42,6 +56,12 @@ public static ValidationRuleSet GetDefaultRuleSet()
            return new ValidationRuleSet(_defaultRuleSet);
        }

+        public static ValidationRuleSet GetEmptyRuleSet()


public [](start = 8, length = 6)

nit, documentation #Closed

PerthCharern · 2018-04-17T17:36:07Z

src/Microsoft.OpenApi.Readers/OpenApiStreamReader.cs

@@ -90,11 +90,14 @@ public OpenApiDocument Read(Stream input, out OpenApiDiagnostic diagnostic)
            }

            // Validate the document
-            var errors = document.Validate(_settings.RuleSet);
-            foreach (var item in errors)
+            if (_settings.RuleSet != null && _settings.RuleSet.Rules.Count > 0)


&& _settings.RuleSet.Rules.Count > 0 [](start = 42, length = 36)

just checking. Is this constraint needed? #Closed

It's there for performance. Otherwise we walk the entire graph and do no validation.

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

PerthCharern · 2018-04-17T17:36:33Z

src/Microsoft.OpenApi.Readers/Services/OpenApiReferenceResolver.cs

@@ -176,7 +177,8 @@ private void ResolveObject<T>(T entity, Action<T> assign) where T : class, IOpen

            if (IsUnresolvedReference(entity))
            {
-                assign(ResolveReference<T>(entity.Reference));
+                var x = ResolveReference<T>(entity.Reference);
+                assign(x);


Why this change? #Closed

I'll revert. I did it temporarily whilst hunting down what was taking so long.

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

PerthCharern · 2018-04-17T17:38:23Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

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


nit, copyright #Closed

PerthCharern · 2018-04-17T17:39:08Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+            };
+
+            // Act
+            var errors = document.Validate(new ValidationRuleSet() { new AllwaysFailRule<OpenApiSchema>()});


AllwaysFailRule [](start = 73, length = 15)

nit, Always typo #Closed

PerthCharern · 2018-04-17T17:39:52Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+
+    }
+
+    public class AllwaysFailRule<P> : ValidationRule<P> where P: IOpenApiElement


P [](start = 33, length = 1)

Not sure what P stands for. Maybe just use the standard T ? #Closed

PerthCharern · 2018-04-17T17:40:57Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+                    [sharedSchema.Reference.Id] = sharedSchema
+                }
+            };
+


Should we also add this schema to an operation like above to test that scenario as well? #Closed

PerthCharern · 2018-04-17T17:41:43Z

src/Microsoft.OpenApi.Workbench/MainWindow.xaml

+               AcceptsReturn="True" 
+               FontFamily="Consolas"
+               VerticalContentAlignment="Stretch" />
+    </DockPanel>


Do you mind posting a screenshot? #Closed

I still don't see the screenshot, but I think it's alright.

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

I put it in the PR description. I wasn't sure where else to put it.

In reply to: 184121152 [](ancestors = 184121152,182167917)

Saw it now. Thanks!

In reply to: 184128635 [](ancestors = 184128635,184121152,182167917)

PerthCharern · 2018-04-17T17:41:55Z

src/Microsoft.OpenApi.Workbench/StatsVisitor.cs

+
+namespace Microsoft.OpenApi.Workbench
+{
+    public class StatsVisitor : OpenApiVisitorBase


public [](start = 4, length = 6)

nit, document public class/properties #Closed

PerthCharern

🕐

darrelmiller · 2018-04-17T18:05:11Z

I considered passing it as a parameter, but as Components can hold many different types, it would require passing that value almost everywhere. Also, we already hold state while walking. We hold the Path in VisitorBase and we hold stacks for tracking loops in the the Walker. It does make me wonder why we have state in both. Maybe the state can be moved into the walker instead of being in the Visitor.

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

darrelmiller · 2018-04-25T04:15:56Z

I was able to move the InComponents state into the walker. We still have the Path Stack in the visitor and the ValidationVisitor holds the list of errors detected. So, I don't think we can make the Visitor completely stateless, but we can at least keep it to a minimum.

In reply to: 382087733 [](ancestors = 382087733,382076651)

PerthCharern · 2018-04-25T16:11:51Z

src/Microsoft.OpenApi/Services/OpenApiWalker.cs

@@ -37,6 +38,9 @@ public void Walk(OpenApiDocument doc)
            {
                return;
            }
+            _schemaLoop.Clear();


nit, newline #Closed

PerthCharern · 2018-04-25T16:12:08Z

Sure. This looks okay to me.

In reply to: 384155854 [](ancestors = 384155854,382087733,382076651)

PerthCharern · 2018-04-25T16:13:50Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+        }
+
+        [Fact]
+        public void UnResolvedReferencedSchemaShouldNotBeValidated()


UnResolvedReferencedSchemaShouldNotBeValidated [](start = 20, length = 46)

nit, casing of unresolved incorrect. also to distinguish this from the test below,

UnresolvedReferencedSchemaInComponentsShouldNotBeValidated #Closed

Fixed the casing issue. One tests is ReferencedSchema, the other is SchemaReference

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

Ah, ok. That makes sense. The other one has a typo though ("Referenced" instead of "Reference")

In reply to: 184126122 [](ancestors = 184126122,184120408)

"Referenced" was intentional, but Reference works too. Changed it.

In reply to: 184126692 [](ancestors = 184126692,184126122,184120408)

PerthCharern · 2018-04-25T16:15:12Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+        }
+
+        [Fact]
+        public void UnResolvedSchemaReferencedShouldNotBeValidated()


UnResolvedSchemaReferencedShouldNotBeValidated [](start = 20, length = 46)

UnresolvedReferenceSchemaInOperationShouldNotBeValidated #Closed

PerthCharern · 2018-04-25T16:18:30Z

    internal void Validate()

nit, "Validate" doesn't seem like the right name for this method. This actually does all the parsing, validating, and collecting the stats.

Maybe focus on the core part of it - which is reading, and call this ReadDocuemnt or ParseDocument? #Closed

Refers to: src/Microsoft.OpenApi.Workbench/MainModel.cs:166 in 7e51f03. [](commit_id = 7e51f03, deletion_comment = False)

PerthCharern · 2018-04-25T16:19:17Z

using System;

nit, copyright #Closed

Refers to: src/Microsoft.OpenApi.Workbench/StatsVisitor.cs:1 in 7e51f03. [](commit_id = 7e51f03, deletion_comment = False)

PerthCharern · 2018-04-25T16:21:03Z

src/Microsoft.OpenApi.Workbench/StatsVisitor.cs

+
+namespace Microsoft.OpenApi.Workbench
+{
+    internal class StatsVisitor : OpenApiVisitorBase


internal class StatsVisitor : OpenApiVisitorBase [](start = 3, length = 49)

I'm not quite sure if this is supposed to be in the workbench. Even if it's a very basic functionality, it seems to me that we can still add it to the core Microsoft.OpenApi as a service provided. Is there a reason you specifically put this visitor here? #Closed

I put it here as a quick and dirty experimental statistics tool. I don't want to commit to something in the core library until we actually spend a bit of time designing it.

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

PerthCharern

darrelmiller · 2018-04-25T16:36:10Z

    internal void Validate()

I renamed it ParseDocument. It's called Convert on the UI but I'm not a fan of that either.

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

Refers to: src/Microsoft.OpenApi.Workbench/MainModel.cs:166 in 7e51f03. [](commit_id = 7e51f03, deletion_comment = False)

PerthCharern · 2018-04-25T17:01:39Z

test/Microsoft.OpenApi.Tests/Validations/OpenApiReferenceValidationTests.cs

+        }
+
+        [Fact]
+        public void UnresolvedReferenceSchemaShouldNotBeValidated()


UnresolvedReferenceSchemaShouldNotBeValidated [](start = 20, length = 45)

I was thinking of the other way round. "Referenced" here and "Reference" below. Sorry I wasn't clear :(

darrelmiller added 3 commits April 13, 2018 18:48

Performance fixes for validation and reference resolution

2988c4b

Updated walker to avoid validating references twice

9436718

Added stats to workbench

b532b1a

darrelmiller requested review from PerthCharern and xuzhg April 16, 2018 00:37

PerthCharern reviewed Apr 17, 2018

View reviewed changes

PerthCharern suggested changes Apr 17, 2018

View reviewed changes

darrelmiller added 2 commits April 24, 2018 23:59

Updates based on code review

e761861

Moved _inComponents logic into Walker

13bf0aa

Merge remote-tracking branch 'origin/master' into dm/perf

7e51f03

PerthCharern reviewed Apr 25, 2018

View reviewed changes

PerthCharern approved these changes Apr 25, 2018

View reviewed changes

Updates based on CR

b4a290c

PerthCharern reviewed Apr 25, 2018

View reviewed changes

darrelmiller merged commit ef6eba9 into master Apr 25, 2018

darrelmiller deleted the dm/perf branch January 4, 2020 20:26


		}

		public class AllwaysFailRule<P> : ValidationRule<P> where P: IOpenApiElement

Performance enhancements to validation and reference resolution #241

Performance enhancements to validation and reference resolution #241

Uh oh!

Conversation

darrelmiller commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern commented Apr 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern Apr 17, 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!

PerthCharern left a comment

Choose a reason for hiding this comment

Uh oh!

darrelmiller commented Apr 17, 2018

Uh oh!

darrelmiller commented Apr 25, 2018

Uh oh!

PerthCharern Apr 25, 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!

PerthCharern commented Apr 25, 2018

Uh oh!

PerthCharern Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

darrelmiller commented Apr 15, 2018 •

edited

Loading

PerthCharern commented Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 17, 2018 •

edited

Loading

PerthCharern Apr 25, 2018 •

edited

Loading

PerthCharern Apr 25, 2018 •

edited

Loading

PerthCharern Apr 25, 2018 •

edited

Loading

PerthCharern commented Apr 25, 2018 •

edited

Loading

PerthCharern commented Apr 25, 2018 •

edited

Loading

PerthCharern Apr 25, 2018 •

edited

Loading