Memory tuning #2619

michaelcfanning · 2023-02-16T00:36:55Z

This change makes some tactical changes to the current threading model in order to maximize CPU utilization and limit memory consumption, while preserving the deterministic output required for certain current client tool users.

@kelewis has pointed out that building analysis around a limited number of flattened records designed to enable efficient results streaming would be much more efficient. This seems like the right approach to me; I am only uncertain about how we would retrofit determinism for those who need it. We could create a post-processing sorting API. We could design a new binary format for serialized records, optimized for reconstructing into sorted SARIF. Looking forward to the conversation.

As it turns out, our runaway memory usage was do to a simple bug, the context creating channel wasn't throttled on write. Oops.

Tactical changes include:

Throttle context creation to, at most, 25k objects. I will make this configurable in the short-term.
Conversion of failure level and result kind to immutable sets, rather than list instances. We now prefer constant values for the default. These allocations were unnecessary and cluttered profiling results, so good to clear.
Added a simple measure of working set consumed at analysis time.
Eliminated HashUtilities.FileSystem, an unnecessary duplication of the FileSystem.Instance test knob. Taking this change required adding an IFileSystem instance to relevant API (a non-breaking change).
Lowered individual 'file skipped due to size' message to note. Generally, we have a couple of issues with diagnostics reporting: these values can't be individually controlled via the failure levels. They likely should be separated from them entirely, as trace values. The trace messages arguably should always be emitted, when configured. Would love to discuss this further with the team.
Updated 'one or more files skipped' warning to include a count of files skipped. Also moved this warning to the end of analysis (rather than embedding it in the middle of analysis results, when file enumeration is complete).
This change aggressively flushes SARIF json to disk, so that a concentration of results doesn't increase memory consumption. This writing occurs on a single thread and the incremental loss of IO efficiency is unlikely to be observeable as it's amortized alongside the multithreaded scanning.
We also now no longer take an exclusive lock on the SARIF log. This allows users to open an in-progress log or us a tool like tail.exe to watch output as it's generated.
I have disabled the caching/replay of file results by hash. This functionality isn't of general value and it definitely shouldn't be tied to the configuration to emit file hashes for files that are referenced in SARIF. This capability will come back, therefore, as a new config knob. In the meantime, we've dropped the hashing channel, which is helpful for lowering memory usage.

… hash.

KalleOlaviNiemitalo · 2023-02-16T12:00:18Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-            // reference to each scan target is put into a channel for hashing,
-            // if hashing is enabled.
-            Task<bool> hashFilesAndPutInAnalysisQueue = Task.Run(() => HashFilesAndPutInAnalysisQueueAsnc());
+            // 3: A dedicated set of threads pull scan targets and analyze them.


Skipped number 2 #Resolved

KalleOlaviNiemitalo · 2023-02-16T12:02:14Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

+            Task.WhenAll(hashWorkers)
+                .ContinueWith(_ => {
+                    _loggerCache?.Clear();
+                    Console.WriteLine("Done with hashing.");
+                    readyToScanChannel.Writer.Complete(); 
+                })
+                .Wait();
+


If some of the tasks in hashWorkers fail, does this ignore the error?

Overall, if you're going to wait anyway, then I feel it would be clearer to use Task.WaitAll and then do _loggerCache?.Clear() etc. afterwards. The error handling would then be different, though.

suvamM · 2023-02-16T17:26:58Z

@michaelcfanning A PR description would be super helpful.

michaelcfanning · 2023-02-21T23:42:47Z

src/Sarif/DefaultTraces.cs

@@ -10,7 +10,7 @@ public enum DefaultTraces
    {
        None,
        ScanTime = 0x01,
-        ScanExecution = 0x2,


ScanExecution

'ScanExecution' previously unused.

michaelcfanning · 2023-02-21T23:49:56Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -30,7 +30,6 @@ public abstract class AnalyzeCommandBase<TContext, TOptions> : PluginDriverComma
        private Run _run;
        private Tool _tool;
        internal TContext _rootContext;
-        private CacheByFileHashLogger _cacheByFileHashLogger;


_cacheByFileHashLogger

I've clobbered caching by file hash in general with this change. It will not come back for the single-threaded analysis, as this engine is getting dropped entirely.

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-
-            return true;
-        }
+        private readonly ConcurrentDictionary<string, CachingLogger> _loggerCache = new ConcurrentDictionary<string, CachingLogger>();


src/Test.UnitTests.Sarif/Writers/BaseLoggerTests.cs

+            baseLoggerTestConcrete = new BaseLoggerTestConcrete(new[] { FailureLevel.Error }.ToImmutableHashSet(),
+                                                                new List<ResultKind> { ResultKind.Informational, ResultKind.Fail }.ToImmutableHashSet());


src/Test.UnitTests.Sarif/Writers/BaseLoggerTests.cs

+            baseLoggerTestConcrete = new BaseLoggerTestConcrete(new[] { FailureLevel.Note }.ToImmutableHashSet(),
+                                                                BaseLogger.Fail);


src/Test.UnitTests.Sarif/Writers/BaseLoggerTests.cs

+            baseLoggerTestConcrete = new BaseLoggerTestConcrete(new[] { FailureLevel.None }.ToImmutableHashSet(),
+                                                                new List<ResultKind> { ResultKind.Informational }.ToImmutableHashSet());


src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

-            Traces = new string[] { };
-            Kind = new List<ResultKind> { ResultKind.Fail };
-            Level = new List<FailureLevel> { FailureLevel.Warning, FailureLevel.Error };
+            Trace = new string[] { };


michaelcfanning · 2023-02-21T23:52:28Z

src/Sarif.Driver/Sdk/AnalyzeOptionsBase.cs

+        public virtual IEnumerable<string> Trace { get; set; }
+
+        private DefaultTraces? defaultTraces;
+        public DefaultTraces Traces


Traces

This pattern may be worth pursuing aggressively, i.e., some class surface area for interacting directly with the commandline library vs. reconstructed data more suitable for direct consumption by the engine.

I keep wondering whether options could be merged entirely with the context object...

michaelcfanning · 2023-02-22T00:02:05Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

-            readyToHashChannel = Channel.CreateBounded<int>(channelOptions);
-
-            channelOptions = new BoundedChannelOptions(2000)
+            var channelOptions = new BoundedChannelOptions(25000)


25000

We allow the scan phase to build up 25k targets, as candidates, or completed files that haven't triggered the logging thread.

This all designed to maximize CPU utilization for the scan phase.

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

+                catch (Exception e)
+                {
+                    if (context != null)
                    {
-                        if (context != null)
-                        {
-                            RuntimeErrors |= context.RuntimeErrors;
-                            context?.Dispose();
-                        }
-                        context = default;
-                        Errors.LogUnhandledEngineException(rootContext, e);
-                        RuntimeErrors |= rootContext.RuntimeErrors;
-                        ThrowExitApplicationException(context, ExitReason.ExceptionWritingToLogFile, e);
+                        RuntimeErrors |= context.RuntimeErrors;
+                        context?.Dispose();
                    }
+                    context = default;
+                    Errors.LogUnhandledEngineException(rootContext, e);
+                    RuntimeErrors |= rootContext.RuntimeErrors;
+                    ThrowExitApplicationException(context, ExitReason.ExceptionWritingToLogFile, e);
+                }


src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -276,62 +278,68 @@
        {
            int currentIndex = 0;

-            ChannelReader<int> reader = _resultsWritingChannel.Reader;
+            while (_fileContexts?.Count > 0 == false)


michaelcfanning added 5 commits February 14, 2023 08:33

Resolve task invocation issue that prevented effective parallelism.

90a2e56

Remove context object from disk enum call.

5edfd3a

Update release notes.

ec93dcc

Merge remote-tracking branch 'origin' into memory-tuning

e01b471

Work in flight for ongoing memory tuning.

8144521

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

Add comment documenting disabling of file caching of results, by file…

e2828ab

… hash.

KalleOlaviNiemitalo reviewed Feb 16, 2023

View reviewed changes

Memory tuning.

5595b31

michaelcfanning commented Feb 21, 2023

View reviewed changes

github-advanced-security bot found potential problems Feb 21, 2023

View reviewed changes

michaelcfanning commented Feb 21, 2023

View reviewed changes

michaelcfanning added 2 commits February 21, 2023 15:57

Remove compute hashes field, as unused.

51b82e5

Remove compute hashes field, as unused.

fe6686d

michaelcfanning commented Feb 22, 2023

View reviewed changes

michaelcfanning added 5 commits February 21, 2023 16:32

Formatting.

79da60e

Merge branch 'main' into memory-tuning

25ced28

Update release notes.

0fe3695

Update to try write in channel use.

ee15385

Add max-targets (default 25k) for limiting memory use.

e2199de

github-advanced-security bot found potential problems Feb 22, 2023

View reviewed changes

michaelcfanning closed this Feb 22, 2023

michaelcfanning mentioned this pull request Feb 24, 2023

New model #2625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory tuning #2619

Memory tuning #2619

michaelcfanning commented Feb 16, 2023 •

edited

Loading

KalleOlaviNiemitalo Feb 16, 2023 •

edited by michaelcfanning

Loading

KalleOlaviNiemitalo Feb 16, 2023

KalleOlaviNiemitalo Feb 16, 2023

suvamM commented Feb 16, 2023

michaelcfanning Feb 21, 2023

michaelcfanning Feb 21, 2023

michaelcfanning Feb 21, 2023

michaelcfanning Feb 22, 2023

		baseLoggerTestConcrete = new BaseLoggerTestConcrete(new[] { FailureLevel.Error }.ToImmutableHashSet(),
		new List<ResultKind> { ResultKind.Informational, ResultKind.Fail }.ToImmutableHashSet());

		baseLoggerTestConcrete = new BaseLoggerTestConcrete(new[] { FailureLevel.Note }.ToImmutableHashSet(),
		BaseLogger.Fail);

Memory tuning #2619

Memory tuning #2619

Conversation

michaelcfanning commented Feb 16, 2023 • edited Loading

KalleOlaviNiemitalo Feb 16, 2023 • edited by michaelcfanning Loading

Choose a reason for hiding this comment

KalleOlaviNiemitalo Feb 16, 2023

Choose a reason for hiding this comment

KalleOlaviNiemitalo Feb 16, 2023

Choose a reason for hiding this comment

suvamM commented Feb 16, 2023

michaelcfanning Feb 21, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 21, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 21, 2023

Choose a reason for hiding this comment

michaelcfanning Feb 22, 2023

Choose a reason for hiding this comment

michaelcfanning commented Feb 16, 2023 •

edited

Loading

KalleOlaviNiemitalo Feb 16, 2023 •

edited by michaelcfanning

Loading