Implement multithreaded driver analyze command w/deterministic output #2146

michaelcfanning · 2020-11-05T16:41:11Z

This change introduces a new MultithreadedAnalyzeCommand that parallelizes analysis while also guaranteeing that all results are output in a deterministic order run-over-run. The mechanism is like so:

A single-threaded enumerator/channel writer enqueues an alphabetized list of directories (based on command-line arguments), then dequeues each directory, enumerates files within each, alphabetizes them and creates an analysis context object for each that is stored in a list. The index of each created context item is written to the channel.
A multithreaded channel reader receives an index for a file to scan, retrieves its corresponding context object from the global array, and analyzes it. Any results are written to the per-scan target context object. On completing analysis, the analysis thread writes the current context index to the results logging channel.
A single-threaded reader receives the index of a context object which represents a scanned target. If the reader has already processed all scan targets leading up to that index, it retrieves the results from the current context and writes them again to the global, actual persisted log (as well as the console). The writer also 'runs ahead' on persisting results if it observes that other threads have completed analysis work which hasn't yet resulted in a channel callback. The writer nulls each context object in the array as work is completed.

To test the above, I've added an 'analyze-test' command to the multitool, which can be a temporary addition until we harden the new code and dropped the single-threaded analysis entirely (this analysis command can eventually be replaced by the multithread analysis configured to run on a single thread). Because we've hit our limit for verbs (as supported by CommandLineLibrary), I had to drop a result matching verb previously used mostly for testing.

Other work and open items:

Most unit tests go through both the single and multi-threaded code paths but not all.
This change includes an optimization to the SarifLogger to stop emitting non-useful artifacts table data when hashing is not enabled.
I discovered an inefficiency in the previous analysis where file hashing is duplicated (i.e., it is performed once when processing scan targets and again the first time any file actually produces a result).

@eddynaka, this is still a draft.

lgtm-com · 2020-11-05T19:47:57Z

This pull request introduces 3 alerts when merging f40a308 into 173eaa0 - view on LGTM.com

new alerts:

3 for Useless assignment to local variable #Resolved

eddynaka · 2020-11-10T10:16:53Z

src/Sarif/Sarif.csproj

+    <Reference Condition="'$(TargetFramework)' == 'net45' Or '$(TargetFramework)' == 'net461' " Include="System.Web" />
+    <Reference Condition="'$(TargetFramework)' == 'net45' " Include="System.Runtime" />


Let's remove this, since we don't target net45 #Resolved

And, for line 38, we should update and add net452 and probably net472. #Resolved

lgtm-com · 2020-11-10T10:21:47Z

This pull request introduces 3 alerts when merging 6ef9b22 into 3f4d953 - view on LGTM.com

new alerts:

3 for Useless assignment to local variable #Resolved

eddynaka · 2020-11-10T10:23:41Z

src/Sarif/Sarif.csproj

+    <Reference Condition="'$(TargetFramework)' == 'net45' Or '$(TargetFramework)' == 'net461' " Include="System.Web" />
+    <Reference Condition="'$(TargetFramework)' == 'net45' " Include="System.Runtime" />


And, for line 38, we should update and add net452 and probably net472. #Resolved

eddynaka · 2020-11-10T10:24:53Z

src/Sarif/HashUtilities.cs

-                            fileToHashDataMap[filePath] = HashUtilities.ComputeHashes(filePath);
+                            HashData hashData = HashUtilities.ComputeHashes(filePath);
+
+                            lock (fileToHashDataMap)


Since we are using a ConcurrentDictionary, probably we won't need the lock. #Resolved

eddynaka · 2020-11-10T10:33:31Z

src/Sarif.Multitool/AnalyzeTestSkimmer.cs

+
+namespace Microsoft.CodeAnalysis.Sarif.Multitool
+{
+    public class AnalyzeTestSkimmer : Skimmer<AnalyzeTestContext>


Since this in a test, should we mark as internal or add a #if debug condition? #Resolved

eddynaka · 2020-11-10T10:33:53Z

src/Sarif.Multitool/AnalyzeTestOptions.cs

+namespace Microsoft.CodeAnalysis.Sarif.Multitool
+{
+    [Verb("analyze-test", HelpText = "Test the analysis driver framework.")]
+    public class AnalyzeTestOptions : MultithreadedAnalyzeOptionsBase


Since this in a test, should we mark as internal or add a #if debug condition? #Resolved

eddynaka · 2020-11-10T10:35:47Z

src/Sarif.Multitool/AnalyzeTestContext.cs

    {
        public Exception TargetLoadException { get; set; }

-        public bool IsValidAnalysisTarget { get; set; }
+        public bool IsValidAnalysisTarget


This could be replaced for this: public bool IsValidAnalysisTarget => true; #Resolved

eddynaka · 2020-11-10T10:36:12Z

src/Sarif.Multitool/AnalyzeTestContext.cs

 // Licensed under the MIT license. See LICENSE file in the project root for full license information.

 using System;
+using System.Collections.Generic;


should we remove those using's that we are not using? #Resolved

eddynaka · 2020-11-10T10:57:29Z

src/Sarif.Multitool/AnalyzeTestCommand.cs

+
+namespace Microsoft.CodeAnalysis.Sarif.Multitool
+{
+    public class AnalyzeTestCommand : MultithreadedAnalyzeCommandBase<AnalyzeTestContext, AnalyzeTestOptions>


should we mark as internal or add a #if debug since its a test? #Resolved

lgtm-com · 2020-11-10T18:41:24Z

This pull request introduces 3 alerts when merging 2de08a1 into 6443453 - view on LGTM.com

new alerts:

3 for Useless assignment to local variable

lgtm-com · 2020-11-10T19:58:12Z

This pull request introduces 2 alerts when merging be28b85 into 6443453 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

lgtm-com · 2020-11-10T20:23:16Z

This pull request introduces 2 alerts when merging c7f4fb7 into 6443453 - view on LGTM.com

new alerts:

2 for Useless assignment to local variable

michaelcfanning and others added 8 commits November 3, 2020 12:47

Add multithreaded analyze command to driver framework.

d86cdec

Merge from master.

5ff74cf

Build and test updates.

882a529

Tests now pass completely.

f00ece2

fixing dotnet-format issues

610b5d6

fixing multitool.library reference in linux

3539cf9

fixing lgtm build

1c8c629

Merge branch 'master' into analyze-test

f40a308

microsoft deleted a comment from lgtm-com bot Nov 6, 2020

eddynaka added 2 commits November 10, 2020 06:48

Merge branch 'master' into analyze-test

594318e

updating package reference version

6ef9b22

eddynaka reviewed Nov 10, 2020

View reviewed changes

Merge branch 'master' into analyze-test

2de08a1

PR feedback.

37711f1

michaelcfanning marked this pull request as ready for review November 10, 2020 19:42

merge from master.

be28b85

michaelcfanning and others added 2 commits November 10, 2020 12:05

Style changes.

639cd28

fixing versionconstant endline

c7f4fb7

eddynaka approved these changes Nov 10, 2020

View reviewed changes

eddynaka merged commit b155c64 into master Nov 10, 2020

eddynaka deleted the analyze-test branch November 10, 2020 20:25

marmegh mentioned this pull request Jul 6, 2022

Resolve unhandled exceptions when analyzing folders that contain large files #2494

Merged

eddynaka mentioned this pull request Aug 15, 2022

Lowering System.Collections.Immutable to 1.5.0 #2533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement multithreaded driver analyze command w/deterministic output #2146

Implement multithreaded driver analyze command w/deterministic output #2146

michaelcfanning commented Nov 5, 2020

lgtm-com bot commented Nov 5, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

lgtm-com bot commented Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 10, 2020

		<Reference Condition="'$(TargetFramework)' == 'net45' Or '$(TargetFramework)' == 'net461' " Include="System.Web" />
		<Reference Condition="'$(TargetFramework)' == 'net45' " Include="System.Runtime" />

Implement multithreaded driver analyze command w/deterministic output #2146

Implement multithreaded driver analyze command w/deterministic output #2146

Conversation

michaelcfanning commented Nov 5, 2020

lgtm-com bot commented Nov 5, 2020 • edited by michaelcfanning Loading

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

lgtm-com bot commented Nov 10, 2020 • edited by michaelcfanning Loading

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

eddynaka Nov 10, 2020 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 10, 2020

lgtm-com bot commented Nov 5, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

lgtm-com bot commented Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading

eddynaka Nov 10, 2020 •

edited by michaelcfanning

Loading