Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH2099: Also matches RFC-02 with a few changes. #2650

Open
wants to merge 3 commits into
base: develop
from

Conversation

@tstewart65
Copy link

tstewart65 commented Oct 27, 2019

This pull request is what started RFC-02. At the time I was not able to submit a pull request for the changes.

Differences from RFC-02

  1. The cache argument was removed. A new section was added to the cake.config file [Cache] with 2 possible properties Enabled and Path. Enabled defaults to false and Path defaults cache under the tools folder.
  2. I originally used a time stamp to determine if the cache was valid or not. This worked for the way I was using Cake but would not work for all cases. I have changed this to use a hash like pull request 2584 used. I did change it to use SHA1CryptoServiceProvider instead of MD5 since MD5 is not FIPS compliant.

I have been using several versions of Cake with this applied and have had no issues.

@mholo65 posted this in the RFC Comments
"Caching compilation is not alone enough. We must also cache registered tools resolved via #tool directive. Addins (and their dependencies) must also be loaded into app domain / load context in same order as the first time when resolved via #addin directive."

For the way this is implemented I feel just caching the compilation is enough. I need all of the lines of the script in order to do the hash for determining if anything has changed. The tools and addins are still being found like usual. The main changes were made in RoslynScriptSession and in there it just determines if it needs to compile the script or run the cached DLL.

@mholo65

This comment has been minimized.

Copy link
Member

mholo65 commented Oct 31, 2019

Related to #2584

@tstewart65 do you possible have some numbers on the performance improvements? Would be nice to see how much time we save with just caching the compilation.

@tstewart65

This comment has been minimized.

Copy link
Author

tstewart65 commented Oct 31, 2019

Results from a very simple cake script that exists of one task and all it does it print out "Hello".
No Cache - 4.008 seconds
Caches - 1.372 seconds

Note: Test are an average of 6 runs with tools and nugget packages already installed.

Next simple test that shows even more saving and uses a version of Cake.Recipe. The test consists of 3 simple tasks but two of the tasks uses the RequireTool call tools.cake. I made a change to the RequiresTool call to use a unique name for each script instead of a GUID so this script could be cached also.
No Cache - 16.493 seconds
Cached - 6.000 Seconds

Real world example is a project that I am currently working on.
No Cache - 31.923 Seconds
Cached - 19.287 Seconds

Below is the Verbose output from the two different basic tests no cache and cached.

Preparing to run build script...
Running Cake bootstrap...
Running build script...
Analyzing build script...
Processing build script...
Compiling build script...
**Script compile time: 1922ms**

========================================
Hello
========================================
Executing task: Hello
Hello
Finished executing task: Hello

========================================
Default
========================================
Executing task: Default
Finished executing task: Default

Task                          Duration
--------------------------------------------------
Hello                         00:00:00.0117959
Default                       00:00:00.0042251
--------------------------------------------------
Total:                        00:00:00.0160210
**build.cake execution time: 2643ms**

(Last Command - 4 seconds, 13 milliseconds)

Preparing to run build script...
Running Cake bootstrap...
Running build script...
Analyzing build script...
Processing build script...
build.cake.dll
**Running cached build script...**

========================================
Hello
========================================
Executing task: Hello
Hello
Finished executing task: Hello

========================================
Default
========================================
Executing task: Default
Finished executing task: Default

Task                          Duration
--------------------------------------------------
Hello                         00:00:00.0122724
Default                       00:00:00.0055681
--------------------------------------------------
Total:                        00:00:00.0178405
**build.cake execution time: 53ms**

(Last Command - 1 second, 389 milliseconds)

Preparing to run build script...
Running Cake bootstrap...
Running build script...
Analyzing build script...
Processing build script...
Installing addins...
build.cake.dll
Cache check failed.
Compiling build script...
**Script compile time: 2406ms**

----------------------------------------
Setup
----------------------------------------
Executing custom setup action...
  ____         _           _____             _
 / ___|  __ _ | | __  ___ |_   _|  ___  ___ | |_
| |     / _` || |/ / / _ \  | |   / _ \/ __|| __|
| |___ | (_| ||   < |  __/  | |  |  __/\__ \| |_
 \____| \__,_||_|\_\ \___|  |_|   \___||___/ \__|


Starting Setup...
Cleaning Project

========================================
Hello
========================================
Executing task: Hello
Hello
Finished executing task: Hello

========================================
VersionTest
========================================
Executing task: VersionTest
Analyzing build script...
Processing build script...
Installing tools...
Compiling build script...
**Script compile time: 2599ms**
**GitVersion.CommandLine_version=5.0.1.cake execution time: 3594ms**
Deleting file D:/CacheTest/Basic_Recipe/GitVersion.CommandLine_version=5.0.1.cake
Version
Finished executing task: VersionTest

========================================
BuildTest
========================================
Executing task: BuildTest
Analyzing build script...
Processing build script...
Installing tools...
Compiling build script...
**Script compile time: 2584ms**
**MSBuild.ExtensionPack_version=4.0.15.cake execution time: 3534ms**
Deleting file D:/CacheTest/Basic_Recipe/MSBuild.ExtensionPack_version=4.0.15.cake
Build
Finished executing task: BuildTest

========================================
CleanProject
========================================
Executing task: CleanProject
Finished executing task: CleanProject

----------------------------------------
Teardown
----------------------------------------
Executing custom teardown action...
Starting Teardown...
Cleaning Project Complete
Deleting nupkg files...
Finished running tasks.

Task                          Duration
--------------------------------------------------
Setup                         00:00:00.0126425
Hello                         00:00:00.0121108
VersionTest                   00:00:04.5714435
BuildTest                     00:00:04.4771106
CleanProject                  00:00:00.0038407
Teardown                      00:00:00.0527423
--------------------------------------------------
Total:                        00:00:09.1298904
**build.cake execution time: 12544ms**

(Last Command - 16 seconds, 506 milliseconds)

Preparing to run build script...
Running Cake bootstrap...
Running build script...
Analyzing build script...
Processing build script...
Installing addins...
build.cake.dll
Running cached build script...

----------------------------------------
Setup
----------------------------------------
Executing custom setup action...
  ____         _           _____             _
 / ___|  __ _ | | __  ___ |_   _|  ___  ___ | |_
| |     / _` || |/ / / _ \  | |   / _ \/ __|| __|
| |___ | (_| ||   < |  __/  | |  |  __/\__ \| |_
 \____| \__,_||_|\_\ \___|  |_|   \___||___/ \__|


Starting Setup...
Cleaning Project

========================================
Hello
========================================
Executing task: Hello
Hello
Finished executing task: Hello

========================================
VersionTest
========================================
Executing task: VersionTest
Analyzing build script...
Processing build script...
Installing tools...
GitVersion.CommandLine_version=5.0.1.cake.dll
**Running cached build script...**
**GitVersion.CommandLine_version=5.0.1.cake execution time: 9ms**
Deleting file D:/CacheTest/Basic_Recipe/GitVersion.CommandLine_version=5.0.1.cake
Version
Finished executing task: VersionTest

========================================
BuildTest
========================================
Executing task: BuildTest
Analyzing build script...
Processing build script...
Installing tools...
MSBuild.ExtensionPack_version=4.0.15.cake.dll
**Running cached build script...**
**MSBuild.ExtensionPack_version=4.0.15.cake execution time: 8ms**
Deleting file D:/CacheTest/Basic_Recipe/MSBuild.ExtensionPack_version=4.0.15.cake
Build
Finished executing task: BuildTest

========================================
CleanProject
========================================
Executing task: CleanProject
Finished executing task: CleanProject

----------------------------------------
Teardown
----------------------------------------
Executing custom teardown action...
Starting Teardown...
Cleaning Project Complete
Deleting nupkg files...
Finished running tasks.

Task                          Duration
--------------------------------------------------
Setup                         00:00:00.0154086
Hello                         00:00:00.0106217
VersionTest                   00:00:00.9394647
BuildTest                     00:00:00.9310428
CleanProject                  00:00:00.0040268
Teardown                      00:00:00.0535480
--------------------------------------------------
Total:                        00:00:01.9541126
**build.cake execution time: 2020ms**

(Last Command - 5 seconds, 895 milliseconds)
Copy link
Member

mholo65 left a comment

Thanks for the PR. Had a quick look and left some thoughts.

@@ -19,11 +19,17 @@ public static class Settings
public const string ShowProcessCommandLine = "Settings_ShowProcessCommandLine";
}

public static class Cache

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

As Cache is a part of Cake, not Cake.Core, these constants should be moved there.

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 22, 2019

Author

Moved to Cake/Constants.cs

/// <param name="defaultRoot">The default root path.</param>
/// <param name="environment">The environment.</param>
/// <returns>The script cache directory path.</returns>
public static DirectoryPath GetScriptCachePath(this ICakeConfiguration configuration, DirectoryPath defaultRoot, ICakeEnvironment environment)

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

Same as with constants. Move this extension to Cake.

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 22, 2019

Author

Moved to Cake/Extensions/CakeConfiguration.cs

/// <summary>
/// Disposable Stopwatch used for timing how long a section of code takes and writing the result to the log.
/// </summary>
public class DisposableStopwatch : IDisposable

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

Don't think we need a stopwatch at this point. Please open separate issue for reporting compilation time.

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 22, 2019

Author

Removed

/// <summary>
/// Optimized hash generator. Using SHA1CryptoServiceProvider since it is FIPS compliant.
/// </summary>
public static class FastHash

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

Hashing is only needed for Caching at this point, move to Cake.

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 22, 2019

Author

Moved to Cake/Utilities/FastHash.cs

@@ -187,6 +187,11 @@ private bool ParseOption(string name, string value, CakeOptions options)
options.Exclusive = ParseBooleanValue(value);
}

if (name.Equals("recompile", StringComparison.OrdinalIgnoreCase))

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

Suggestion for better name? invalidate-cache or similar. Something that refers to cache would be good.

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 22, 2019

Author

Change "recompile" to "regeneratecache"

private readonly IAssemblyLoader _loader;
private readonly ICakeLog _log;

public RoslynScriptEngine(CakeOptions options, IAssemblyLoader loader, ICakeLog log)
public RoslynScriptEngine(CakeOptions options, ICakeConfiguration configuration, IAssemblyLoader loader, ICakeLog log)

This comment has been minimized.

Copy link
@mholo65

mholo65 Nov 20, 2019

Member

@patriksvensson if I remember correctly, you where removing places where we pass IConfiguration around in your CLI rewrite PR?

This comment has been minimized.

Copy link
@tstewart65

tstewart65 Nov 20, 2019

Author

Yes this part is not needed when using the CLI review RFC0001. I have also made the changes for the cache using the RFC0001 pull request.

break;
_log.Verbose(cacheDLLFileName);
scriptHash = FastHash.GenerateHash(Encoding.UTF8.GetBytes(string.Concat(script.Lines)));
var cachedHash = IO.File.Exists(hashFile.FullPath) ? IO.File.ReadAllText(hashFile.FullPath) : string.Empty;

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Don't use System.IO use the IFileSystem

This comment has been minimized.

Copy link
@tstewart65
if (_scriptCacheEnabled)
{
// Verify cache directory exists
if (!IO.Directory.Exists(_scriptCachePath.FullPath))

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Don't use System.IO use the IFileSystem

This comment has been minimized.

Copy link
@tstewart65
// Verify cache directory exists
if (!IO.Directory.Exists(_scriptCachePath.FullPath))
{
IO.Directory.CreateDirectory(_scriptCachePath.FullPath);

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Don't use System.IO use the IFileSystem

This comment has been minimized.

Copy link
@tstewart65

private void RunScriptAssembly(string assemblyPath)
{
var assembly = Assembly.LoadFile(assemblyPath);

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Use IAssemblyLoader here

This comment has been minimized.

Copy link
@tstewart65
roslynScript.RunAsync(_host).Wait();
try
{
var task = (System.Threading.Tasks.Task<object>)factoryMethod.Invoke(null, new object[] { new object[] { _host, null } });

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

No need for System.Threading.Tasks. Include the namespace instead.

This comment has been minimized.

Copy link
@tstewart65

if (emitResult.Success)
{
IO.File.WriteAllText(hashFile.FullPath, scriptHash);

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Don't use System.IO use the IFileSystem

This comment has been minimized.

Copy link
@tstewart65
{
IO.Directory.CreateDirectory(_scriptCachePath.FullPath);
}
if (string.IsNullOrEmpty(scriptHash))

This comment has been minimized.

Copy link
@patriksvensson

patriksvensson Nov 21, 2019

Member

Use IsNullOrWhitespace

This comment has been minimized.

Copy link
@tstewart65
@tstewart65 tstewart65 requested review from patriksvensson and mholo65 Nov 22, 2019
@gep13 gep13 changed the title Fix for #2099. Also matches RFC-02 with a few changes. GH2099: Also matches RFC-02 with a few changes. Nov 29, 2019
tstewart65 added 3 commits Oct 27, 2019
Differences from RFC-02
1) The cache argument was removed.  A new section was added to the cake.config file [Cache] with 2 possible properties Enabled and Path.  Enabled defaults to false and Path defaults cache under the tools folder.
2) I originally used a time stamp to determine if the cache was valid or not.  I have changed this to use a hash like pull request 2584 used.  I did change it to use SHA1CryptoServiceProvider instead of MD5 since MD5 is not FIPS compliant.
…ssembly.Load call.
@devlead devlead force-pushed the tstewart65:feature/ScriptCache branch from 18f1cbd to 04d2e50 Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.