-
Notifications
You must be signed in to change notification settings - Fork 0
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
HUDL - SPIKE - Adds output caching to cake runtime #1
Conversation
It seems like with this if you make code changes to the cake files, you would need to run it |
yeah i thought about that and i figured that for us at least that would only impact folks developing on the scripts, end users wouldn't really care or would rarely need to recompile IMO. We could compute a hash of the input file contents and use that as part of the path to the compiled assembly. That might add some complexity to the code since we'd have to compute the hash in one section and pass it down into the roslyn generator. I'm not sure we'd need to do this for Hudls use, but it's something that seems necessary before we submit back to cake proper. |
I found this RFC posted in their repo about assembly caching. You might want to check it to see if there's anything that would need to be different to match it. I'm guessing they wouldn't accept the change if it doesn't follow the RFC. |
Yeah i did see the RFC -> https://github.com/cake-build/rfcs/pull/2/files#diff-9b01d3179a039b60e4fcdb5e273d48c2 and read through it, I think we're following it pretty closely already, except for the recompile issue you mentioned and the fact that they would want compilation to be opt-in rather than default like it is in our case here. Those two changes could be made pretty easily,
|
Well, one thing I could see happening is:
|
Oh, yeah thats a good case. One other thing is this comment on the RFC: https://github.com/cake-build/rfcs/pull/2/files#r229862533
We're doing some of this now, all the DLLs that are references in the compilation are stored with the cached dll, but we're not loading them in a particular order, that hasn't seemed to matter in our usage, but if it did i could see that being complicated, and probably requiring us to rename the files or find some other way to enforce their ordering. |
@@ -30,6 +30,10 @@ internal sealed class RoslynScriptSession : IScriptSession | |||
|
|||
public HashSet<string> Namespaces { get; } | |||
|
|||
public bool SupportsCachedExecution => true; | |||
|
|||
public bool IsCacheValid => System.IO.File.Exists(GetCachedAssemblyPath()) && !_options.ForceRecompile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like very aggressive caching. Unless I'm missing something, if a newer version of Cake isn't installed, updates to the dev scripts will get ignored unless the explicit force recompile option is passed. How do we ensure updates take effect on a dev's machine?
Could we time how long the analysis step takes? It feels like a hash of the result from this could be a more robust stage to cache at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah @jamiegs was just mentioning this. I think we can compute a hash of the files much like we do in CICD for output caching. I think the analyzer does take some time to run, getting a benchmark of it would be a good next step.
cache is invalidated by file content changes
The cache is now invalidated by cake script changes. This adds ~2s to the execution time in my testing (because the analyzer has to run and hash all the script files). Still, reducing pre-script "build time" from 10s to 3s is still pretty impressive. I updated the code so that the cache can be enabled via the built-in cake configuration, either with
Default values for both of these are |
This is great work Jared! I'm going to give it a try today. I'm curious if just checking file modification times against the time the assembly was built would be good enough to invalidate the cache. That should be less expensive than hashing. |
I've run into 3 problems
And all of that was on develop... :/ EDIT: I'm getting error EDIT 2: |
After a successful build of Cake-bin-coreclr-v0.35.0-HUDL-SpikeAddAss0001 and putting it into hudl-microservicetemplate and unzipping it to the old 0.33.0 folder I don't see any speed improvements. As per the discussion above, I think checking the file modification date should be enough. |
The hashing portion of the code doesnt add much to the execution time since cake already compiles a list containing the lines for each script file. I can benchmark checking the most recent modification date against the dll date |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this and I do have an issue where AWSDK.Core failes to load, when using the cached script.dll. However this caching approach is definitely worth pursuing further. It feels much nicer to get quick feedback when the tool is running.
From chatting with the team this morning it seems there might be a few more kinks to work out in the PR before we finalize:
|
This is looking like it's in a good place. I'm going to close this and open a new PR against the parent project, i can't see another way to do this within the GitHub ui. |
This updates the Roslyn script generator to emit the generated code to a DLL (taking some of the code from cake-build#2099 as inspiration). The DLL is stored in
.cache/
underneath the directory the Cake.dll resides in.If the cached assembly exists it will be loaded and executed, unless the
--recompile
flag is sent as an arg into cake.Results
I ran the new cake version against hudl-microservicetemplate and recorded the execution times of a fresh run and a cached run. FYI the paths still show cake 0.33.0 but that is just because I replaced the cake files there with the built ones from this branch (see "How do I test this?" below).
Fresh run
Cached run
How do I test this?
./build.ps1
from the root./artifacts/v0.35.0-HUDL-SpikeAddAss0001/Cake-bin-coreclr-v0.35.0-HUDL-SpikeAddAss0001.zip
file and replace the contents ofcake.coreclr
in your marvel projectstools
directory. Then you can rundotnet <path/to/cake.dll> <path/to/build.cake> --target=<TARGET> --verbosity=Diagnostic --cache_enabled=true
to run the new cake version with the verbose logs turned on for more context.Updates to cache invalidation
The cache is now invalidated by cake script changes. This adds ~2s to the execution time in my testing (because the analyzer has to run and hash all the script files). Still, reducing pre-script "build time" from 10s to 3s is still pretty impressive.
I updated the code so that the cache can be enabled via the built-in cake configuration, either with
CAKE_CACHE_ENABLED=true
andCAKE_CACHE_FORCE_RECOMPILE=true
environment variables,--cache_enabled=true --cache_force_recompile=true
args, or by adding the following to the cake.config:Default values for both of these are
false
.