New model #2625

michaelcfanning · 2023-02-22T21:23:57Z

Foundational work for updated threading model that better utilizes CPU while throttling memory use and allows for cancellation and timeout. The code is refactored to allow API consumers to plug-in at the highest level possible (rather than calling into individual methods to instantiate skimmers, analyze targets, etc.). Consumers can drive analysis strictly from command-line options (mirroring client tool behavior closely) or simply create a new (greatly expanded) context object that is sufficient to drive end-to-end analysis.

As a result of the context work, all parameterization can be expressed as an XML configuration file as well.

BRK: MultithreadedAnalyzeCommandBase IDispose implementation now manages logging dispose. Be sure to call base.Dispose() in any derived type implementations. #2614
BRK: Rename LogFilePersistenceOptions to FilePersistenceOptions (due to its general applicability in other file persistence contexts other than output logs).#2625
BRK: Many breaking changes in IAnalysisContext and AnalyzeContextBase. #2625
BUG: Eliminate per-context allocations contributing to unnecessary memory use. #2625
NEW: Rewrite MultithreadedAnalyzeCommandBase pipeline to allow for timeout, cancellation, and better API-driven use. #2625
NEW: Move large amounts of scan data to the context object, to streamline pipeline and allow for XML-driven configuration. #2625
NEW: Switch file processing to an ArtifactProvider model where enumerated artifacts consist of URI and optional content. #2625
NEW: Add new FailureLevelSet and ResultKindSet types that are compatible with XML-based configuration. #2625
NEW: Add PeakWorkingSet to --trace command to report maximum working set value during analysis. #2619

This is a large change and it's going in with many TBD comments I will address next week. Everyone will have further opportunity to review as it goes into sarif-pattern-matcher, where the new pipeline is highly exercised.

…der.

…leName/Path extenson method.

michaelcfanning · 2023-02-24T21:54:01Z

src/Sarif/AggregatingArtifactsProvider.cs

+
+namespace Microsoft.CodeAnalysis.Sarif
+{
+    public class AggregatingArtifactsProvider : IArtifactProvider


AggregatingArtifactsProvider

An aggregating artifacts provider is an artifacts provider that itself aggregates scan targets generated by one or more providers. Directly analogous to our aggregating logger.

Note that individual providers own problems such as skipping files which aren't appropriate for scanning (because, for example, they exceed a size threshold) and for reporting those that were skipped.

michaelcfanning · 2023-02-24T21:56:12Z

src/Sarif/AnalyzeContextBase.cs

@@ -16,42 +19,254 @@ public AnalyzeContextBase()
        // All of these properties are persisted to configuration XML and can be
        // passed using that mechanism. All command-line arguments are 
        // candidates to follow this pattern.
-        public IEnumerable<IOption> GetOptions()
+        public virtual IEnumerable<IOption> GetOptions()


GetOptions

This is the way that a class declares it has serializable properties. This array is not complete.

An 'export-config' command will use MEF to retrieve all instances of IOptionsProvider and subsequently dump all published properties to an XML representation. What's serialized is what's expressed in the underlying static descriptors.

michaelcfanning · 2023-02-24T21:57:31Z

src/Sarif/AnalyzeContextBase.cs

+        public bool PrettyPrint => OutputFileOptions.HasFlag(FilePersistenceOptions.PrettyPrint);
+        public bool ForceOverwrite => OutputFileOptions.HasFlag(FilePersistenceOptions.ForceOverwrite);
+
+        public virtual string AutomationId


AutomationId

This is the way we create class 'surface area' for properties that are backed by the policy property bag. That dictionary can be kept empty, in which case any retrieval of the static gets the default value from the static descriptor.

This also means that the policy dictionary only contains data values that override defaults, which allows for very compact serialized representation.

michaelcfanning · 2023-02-24T22:01:03Z

src/Sarif/EnumeratedArtifact.cs

+
+namespace Microsoft.CodeAnalysis.Sarif
+{
+    public class EnumeratedArtifact : IEnumeratedArtifact


EnumeratedArtifact

EnumeratedArtifact is a thing.

It may exist only as a URI reference, in which case the URI can be resolved to retrieve it.

It can be populated with a stream, in which case there's no need to retrieve/download/load it.

It can be pre-populated with a string representation. We allow this because in the majority of cases we are in the business of analyzing strings. We might need to reconsider this approach, though, as other scan targets won't always be a string.

Probably, this type needs to be generic, with EnumeratedArtifact ultimately resolving to a string.

michaelcfanning · 2023-02-24T22:02:57Z

src/Sarif/Errors.cs

@@ -84,7 +88,7 @@ public static void LogExceptionLoadingTarget(IAnalysisContext context)
            // Could not instantiate skimmers from the following plugins: {0}
            context.Logger.LogConfigurationNotification(
                CreateNotification(
-                    context.TargetUri,


TargetUri

there is no target uri for this and some other error handlers i cleaned up.

michaelcfanning · 2023-02-24T22:07:57Z

src/Sarif/ExtensionMethods.cs

@@ -190,15 +190,16 @@ public static string ToAndPhrase(this List<string> words)
            }
        }

-        public static OptionallyEmittedData ToFlags(this IEnumerable<OptionallyEmittedData> optionallyEmittedData)
+        public static TFlags ToFlags<TFlags>(this IEnumerable<TFlags> enumeratedFlags) where TFlags : Enum, IConvertible


ToFlags

made this work for any enum type that is int32

michaelcfanning · 2023-02-24T22:08:23Z

src/Sarif/FailureLevelSet.cs

+{
+    [Serializable]
+    [JsonConverter(typeof(TypedPropertiesDictionaryConverter))]
+    public class FailureLevelSet : HashSet<FailureLevel>


FailureLevelSet

Enable clean serialization of this set type to and from XML.

Note that we could make these sets immutable, could be desirable.

michaelcfanning · 2023-02-24T22:09:23Z

src/Sarif/Notes.cs

                    Msg002_FileSkippedDueToSize,
-                    ruleId: Msg002_FileSkippedDueToSize,
+                    ruleId: null,


null

oops, this is an associated rule id, not the notification id, and should have been null.

michaelcfanning · 2023-02-24T22:09:59Z

src/Sarif/PropertiesDictionaryExtensionMethods.cs

@@ -95,6 +95,20 @@ public static class PropertiesDictionaryExtensionMethods
                    continue;
                }

+                FailureLevelSet failureLevelSet = property as FailureLevelSet;


failureLevelSet

changes in this file allow for clean serialization and deserialization of sets that contain failure levels and result kinds, highly used to configure analysis.

michaelcfanning · 2023-02-24T22:11:29Z

src/Sarif/Core/Notification.cs

-            sb.Append(" : ").Append(this.Descriptor.Id);
-            sb.Append(" : ").Append(this.AssociatedRule?.Id);
-            sb.Append(" : ").Append(this.Level);
+            if (this.TimeUtc != default) { sb.Append($"{this.TimeUtc}"); }


this

Provides a very nice way in the debugger to see what unexpected stuff is getting persisted to configuration and tool notifications, often something like an unhandled engine exception.

michaelcfanning · 2023-02-24T22:12:22Z

src/Sarif/Writers/ConsoleLogger.cs

@@ -15,7 +15,7 @@ public class ConsoleLogger : BaseLogger, IAnalysisLogger
        //  TODO:  We directly instantiate this logger in two classes, creating 
        //  unamanged dependencies.  Fix this pattern with dependency injection or a factory.
        //  #2272 https://github.com/microsoft/sarif-sdk/issues/2272
-        public ConsoleLogger(bool quietConsole, string toolName, IImmutableSet<FailureLevel> levels = null, IImmutableSet<ResultKind> kinds = null) : base(levels, kinds)


IImmutableSet

immutable set can't easily be serialized/deserialized, so we create alias types to these constructs.

The option was removed before the 4.1.0 release [1], unfortunately the documentation wasn't updated until significantly later [2]. [1] microsoft/sarif-sdk#2625 [2] microsoft/sarif-sdk#2656

michaelcfanning added 30 commits January 22, 2023 10:17

Start of significant pipeline redesign for improved API-driven use.

57eefc3

Resolve test breaks.

31c6ecc

All tests pass.

68a31ff

More test fixes.

b10335d

Start providing facility for tracking skipped files in artifact provi…

682e32c

…der.

Merge from main.

f7549f2

Ongoing architectural and test updates for new pipeline model.

9988a4d

Formatting updates.

e8faa40

More work for unit-testing.

44622c6

Merge remote-tracking branch 'origin' into pipe-redesign

b964f40

Remove instance data from analyze command base

a96d837

Update logger dispose pattern to be driven by context dispose.

3e71ee7

Eliminate single-threaded analysis command base.

ab01d58

Update release history and move location.

78f818b

Update release history for single threaded engine removal.

c600ffb

Restore build after converting various URL dereferences to call GetFi…

9222a94

…leName/Path extenson method.

Clean up dispose pattern for validation command.

38f99d3

Update package build for release history move.

9a26c21

Update various references to moved release history markdown file.

026521f

Formatting changes.

02205cb

Code clean-up in support of broader driver functionality.

892fc21

All tests pass after recent pipeline updates (to flow context, mostly).

3cec429

Continue converging functionality on the context object.

268acb4

All tests pass with ongoing pipeline redesign.

6296ac9

Formatting.

1be9ed7

Updates for upstream consumers for build.

da948fd

Add rich return code to context.

3f6f3e6

Collapse new pipeline model into single method with options and context.

3fa7853

Add explicit return error for command-line processing issue.

ab92529

Allow null context to LogUnhandedEngineException.

cd27269

michaelcfanning added 3 commits February 23, 2023 17:35

Ready for new model mvp.

5e6077c

Update release notes.

3bc68cf

Add new set classes.

708d436

michaelcfanning marked this pull request as ready for review February 24, 2023 16:21

michaelcfanning requested review from EasyRhinoMSFT, marmegh and cfaucon as code owners February 24, 2023 16:21

michaelcfanning added 3 commits February 24, 2023 08:26

Update release notes.

c740033

Fix test breaks.

bd91e0b

All tests pass.

e4e0e3d

michaelcfanning commented Feb 24, 2023

View reviewed changes

michaelcfanning added 3 commits February 24, 2023 14:13

Small updates after code review.

c236cd3

Make traces test windows only.

a9a8813

Disable misbehaving test.

212e422

michaelcfanning enabled auto-merge (squash) February 24, 2023 23:09

michaelcfanning merged commit a29f202 into main Feb 24, 2023

michaelcfanning deleted the new-model branch February 24, 2023 23:13

airtower-luna mentioned this pull request Apr 22, 2023

Replace --force, removed in sarif-multitool 4.1.0 airtower-luna/convert-to-sarif#10

Merged

rwoll mentioned this pull request Jul 14, 2024

chore: remove unused coyote bits #2816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New model #2625

New model #2625

michaelcfanning commented Feb 22, 2023 •

edited

Loading

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

michaelcfanning Feb 24, 2023

New model #2625

New model #2625

Conversation

michaelcfanning commented Feb 22, 2023 • 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

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

michaelcfanning commented Feb 22, 2023 •

edited

Loading