C#: Early identification of duplicate extraction #1851

hvitved · 2019-08-30T14:03:01Z

This change makes a big impact on CoreFX and Mono:

Project	Build + extraction time before	Build + extraction time after	Factor	Same DB (# source files + size)
CoreFX	02:07:03	01:07:38	0.5323363505	yes
Mono	03:26:02	01:31:02	0.4418378903	yes
EntityFrameworkCore	00:21:10	00:22:06	1.044094488	yes
CoreCLR	00:06:35	00:06:52	1.043037975	yes
OneOf	00:05:39	00:06:02	1.067846608	yes
Powershell	00:06:43	00:06:58	1.037220844	yes
OrchardCore	00:33:02	00:33:42	1.020181635	yes
ql	00:02:55	00:02:46	0.9485714286	yes
roslyn	01:04:43	01:04:31	0.996909606	yes

calumgrant

Nice sleuthing. This is an extremely reasonable solution. I'm sorry that it wasn't clear enough that the args-files should have been the verbatim command line arguments that can be passed by @ to the extractor command line.

calumgrant · 2019-08-30T14:25:35Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

+        /// Logs information about the extractor, as well as the arguments to Roslyn.
+        /// </summary>
+        /// <param name="roslynArgs">The arguments passed to Roslyn.</param>
+        /// <returns>A Boolean indicating whether to proceed with extraction.</returns>


Perhaps, "A Boolean indicating whether the same arguments have been logged previously." The decision about extraction is taken elsewhere.

calumgrant · 2019-09-13T14:50:06Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs


            layout = new Layout();
            this.options = options;

-            extractor = new Extraction.Extractor(false, GetOutputName(compilation, commandLineArguments), Logger);
+            finalizeInit = comp =>


I think it would be clearer to just pull this out into a method FinalizeInitialization(CSharpCompilation comp ...), and to remove the finalizeInit argument on line 51. Then just call FinalizeInitialization explicitly.

calumgrant · 2019-09-13T14:52:17Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

@@ -259,7 +260,7 @@ void DoAnalyseAssembly(PortableExecutableReference r)
                var projectLayout = layout.LookupProjectOrDefault(assemblyPath);
                using (var trapWriter = projectLayout.CreateTrapWriter(Logger, assemblyPath, true, options.TrapCompression))
                {
-                    var skipExtraction = FileIsCached(assemblyPath, trapWriter.TrapFile);
+                    var skipExtraction = options.Cache && File.Exists(trapWriter.TrapFile);


Optionally, push logic into FileIsCached.

FileIsCached actually does more, it also checks for timestamps, which is what didn't work for Mono and CoreFX.

I meant, change FileIsCached to use File.Exists and not timestamps.

csharp/extractor/Semmle.Extraction.CSharp/Extractor.cs

calumgrant · 2019-09-13T15:00:25Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

            {
-                Logger.Log(Severity.Info, $"  Arguments to Roslyn: {string.Join(' ', roslynArgs)}");
+                streamWriter.WriteLine($"Arguments to Roslyn: {string.Join(' ', roslynArgs)}");


I don't think we want this, because we want to be able to @-include the arguments file on the command line arguments for debugging purposes, if we need to reproduce extraction of a particular compilation command.

The reason it is there is because we need the hash of the file to include all arguments to Roslyn, not just the ones from the response file, in order to identify identical compilations. However, it appears that response files can contain comments, so adding this line as a comment should work.

The comment worked beautifully, nice find. However, the hash of the file and the contents of the file could be different. You hash all the "inputs" (i.e. the command line), and could even have an empty file just to indicate that the arguments have been seen.

calumgrant · 2019-09-17T08:47:21Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

@@ -259,7 +260,7 @@ void DoAnalyseAssembly(PortableExecutableReference r)
                var projectLayout = layout.LookupProjectOrDefault(assemblyPath);
                using (var trapWriter = projectLayout.CreateTrapWriter(Logger, assemblyPath, true, options.TrapCompression))
                {
-                    var skipExtraction = FileIsCached(assemblyPath, trapWriter.TrapFile);
+                    var skipExtraction = options.Cache && File.Exists(trapWriter.TrapFile);


I meant, change FileIsCached to use File.Exists and not timestamps.

calumgrant · 2019-09-17T08:50:35Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

            {
-                Logger.Log(Severity.Info, $"  Arguments to Roslyn: {string.Join(' ', roslynArgs)}");
+                streamWriter.WriteLine($"Arguments to Roslyn: {string.Join(' ', roslynArgs)}");


The comment worked beautifully, nice find. However, the hash of the file and the contents of the file could be different. You hash all the "inputs" (i.e. the command line), and could even have an empty file just to indicate that the arguments have been seen.

calumgrant

A small suggestion but the overall approach LGTM.

calumgrant · 2019-09-19T11:30:25Z

csharp/extractor/Semmle.Extraction.CSharp/Analyser.cs

@@ -456,7 +456,7 @@ public bool LogRoslynArgs(string[] roslynArgs, string extractorVersion)
            bool argsWritten;
            using (var streamWriter = new StreamWriter(new FileStream(tempFile, FileMode.Append, FileAccess.Write)))
            {
-                streamWriter.WriteLine($"# Arguments to Roslyn: {string.Join(' ', roslynArgs)}");
+                streamWriter.WriteLine($"# Arguments to Roslyn: {string.Join(' ', roslynArgs.Where(arg => arg[0] != '@'))}");


!arg.StartsWith("@")

and fix CommandLineExtensions.cs:19 as well?

It's just possible that arg is empty.

Yeah, that is better.

hvitved added the C# label Aug 30, 2019

hvitved force-pushed the csharp/early-identify-duplicate-extraction branch from a68bad1 to a369db9 Compare September 2, 2019 08:04

hvitved force-pushed the csharp/early-identify-duplicate-extraction branch 3 times, most recently from f9260b1 to e63e791 Compare September 11, 2019 14:06

C#: Early identification of duplicate extraction

8f3f940

hvitved force-pushed the csharp/early-identify-duplicate-extraction branch from e63e791 to 8f3f940 Compare September 11, 2019 18:47

hvitved marked this pull request as ready for review September 12, 2019 09:41

hvitved requested a review from a team as a code owner September 12, 2019 09:41

hvitved requested a review from calumgrant September 12, 2019 09:41

calumgrant suggested changes Sep 13, 2019

View reviewed changes

C#: Address review comments

4f897b2

calumgrant previously approved these changes Sep 17, 2019

View reviewed changes

hvitved dismissed calumgrant’s stale review via 804521d September 19, 2019 07:54

calumgrant reviewed Sep 19, 2019

View reviewed changes

hvitved force-pushed the csharp/early-identify-duplicate-extraction branch from 804521d to 61bd9f2 Compare September 19, 2019 11:39

C#: Address review comments

61bd9f2

calumgrant approved these changes Sep 19, 2019

View reviewed changes

semmle-qlci merged commit 0387177 into github:master Sep 19, 2019

hvitved deleted the csharp/early-identify-duplicate-extraction branch September 19, 2019 18:45

This was referenced Sep 30, 2019

C#: Cleanup more files after failed autobuilder attempt #2054

Merged

C#: Prepend enclosing method in local function TRAP labels #2061

Merged

aliscco mentioned this pull request Sep 24, 2022

[Snyk] Fix for 8 vulnerabilities aliscco/codeql#34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C#: Early identification of duplicate extraction #1851

C#: Early identification of duplicate extraction #1851

Uh oh!

hvitved commented Aug 30, 2019 •

edited

Loading

Uh oh!

calumgrant left a comment

Uh oh!

calumgrant Aug 30, 2019

Uh oh!

calumgrant Sep 13, 2019

Uh oh!

calumgrant Sep 13, 2019

Uh oh!

hvitved Sep 16, 2019

Uh oh!

calumgrant Sep 17, 2019

Uh oh!

Uh oh!

calumgrant Sep 13, 2019

Uh oh!

hvitved Sep 16, 2019

Uh oh!

calumgrant Sep 17, 2019

Uh oh!

calumgrant Sep 17, 2019

Uh oh!

calumgrant Sep 17, 2019

Uh oh!

calumgrant left a comment

Uh oh!

calumgrant Sep 19, 2019 •

edited

Loading

Uh oh!

hvitved Sep 19, 2019

Uh oh!

Uh oh!

C#: Early identification of duplicate extraction #1851

C#: Early identification of duplicate extraction #1851

Uh oh!

Conversation

hvitved commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calumgrant left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

calumgrant left a comment

Choose a reason for hiding this comment

Uh oh!

calumgrant Sep 19, 2019 • 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!

Uh oh!

hvitved commented Aug 30, 2019 •

edited

Loading

calumgrant Sep 19, 2019 •

edited

Loading