emgarten
commented
7 years ago Open terrajobst opened 7 years ago
emgarten
commented
7 years ago 
terrajobst
commented
7 years ago Yeah, I'm not a fan of an explicit clean as the sole mechanism. We have other discussions on how to hide package restore even more by making dotnet build behave like VS, i.e. running restore if necessary.
I think having a nuget gc command as @davidfowl suggested would be OK, but I really really want nuget restore to apply some cache expiration policy automatically. It doesn't have to be perfect, but something that doesn't fill up my drive indefinitely unless I invoke some special command.
anangaur
commented
6 years ago @terrajobst You still would need some process to clean the cache up? I think and explicit command to purge is the best way. Some of the requirements:
Today we do not keep any metadata around source, signature or lastUsed information. May be we should start persisting these info.
terrajobst
commented
6 years ago The way I'd design the cache is like this:
Not sure (1) is viable for perf reasons. If it's not, only bump the dates every so often.
The meta point is that a cache without expiration control isn't a cache -- it's a leak.
anangaur
commented
6 years ago Perf is one consideration. restore could just touch the file (like in linux touch command) which should be more perfomant (or my belief :) ). But I am still not sure which command/process does the clean up i.e. delete of the expired or old packages from the cache. Again restore cannot be the one to do this (performance criterion).
terrajobst
commented
6 years ago It has to be automatic which means it has to be part of restore.
anangaur
commented
6 years ago Ideally yes but without impacting restore perf :)
terrajobst
commented
6 years ago Agreed, but it seems there must be a compromise between spamming my drive and impacting my package restore times.
Maybe we can address 90% of this if .NET Core wouldn’t be modeled as a large number of packages though.
anangaur
commented
6 years ago Maybe we can address 90% of this if .NET Core wouldn’t be modeled as a large number of packages though.
@terrajobst - what's the current thinking here? Is there anything beyond early discussion in this direction? All ears.
terrajobst
commented
6 years ago It’s all early discussions for .NET Core 3.0
dotMorten
commented
5 years ago FWIW I put this little console app in a scheduled task. It deletes any package not accessed the past 30 days:
using System;
using System.IO;
using System.Linq;
namespace NugetCacheCleaner
{
class Program
{
static void Main(string[] args)
{
int minDays = 30;
if (args.Length > 0 && int.TryParse(args[0], out minDays)) { }
string nugetcache = $"{Environment.GetFolderPath(Environment.SpecialFolder.UserProfile)}\\.nuget\\packages\\";
DirectoryInfo info = new DirectoryInfo(nugetcache);
long totalDeleted = 0;
foreach (var folder in info.GetDirectories())
{
foreach (var versionFolder in folder.GetDirectories())
{
var folderInfo = versionFolder.GetFiles("*.*", SearchOption.AllDirectories).GroupBy(i => 1).Select(g => new { Size = g.Sum(f => f.Length), LastAccessTime = g.Max(f => f.LastAccessTime) }).First();
var lastAccessed = DateTime.Now - folderInfo.LastAccessTime;
if (lastAccessed > TimeSpan.FromDays(minDays))
{
Console.WriteLine($"{versionFolder.FullName} last accessed {Math.Floor(lastAccessed.TotalDays)} days ago");
try
{
versionFolder.MoveTo(Path.Combine(versionFolder.Parent.FullName, "_" + versionFolder.Name)); //Attempt to rename before deleting
versionFolder.Delete(true);
totalDeleted += folderInfo.Size;
}
catch { }
}
}
if (folder.GetDirectories().Length == 0)
folder.Delete(true);
}
var mbDeleted = (totalDeleted / 1024d / 1024d).ToString("0");
Console.WriteLine($"Done! Deleted {mbDeleted} Mb");
}
}
}If you're lazy like me, you might want to use @dotMorten's code through this dotnet global tool: https://github.com/terrajobst/dotnet-nuget-gc
Liversage
commented
5 years ago A bit tangential to this issue but I would like to make a comment on the placement of the NuGet cache on the file system.
The cache is inside the ~/.nuget folder and the developers making this decision must have been using a Unix-like operating system to pick this name. However, I would like to direct your attention to another operating system popular among .NET developers: Microsoft Windows. 😉
On Windows the tilde ~ doesn't have a special meaning in the "Windows shell". However, Windows has the concept of known folders and ~ on Unix is the same as the known folder FOLDERID_Profile or as an environment variable %USERPROFILE% which typically is C:\Users\UserName.
While Microsoft is providing less and less specific guidelines on how to use these known folders I would be so bold as to say that applications should not create their own folder in the user profile folder. This is the user's space and dumping application data here is not a good user experience. Also, adding a dot in front of a folder does not make any sense on Windows. These folders are by default hidden on Unix-like operating systems but not so on Windows. In fact, you are unable to use Windows Explorer to create or rename a folder when the name of the folder starts with a dot.
The correct placement of a cache on Windows is in the subfolder vendor\product in FOLDERID_LocalAppData which typically is the folder %USERPROFILE%\AppData\Local so the resulting folder would be something like %USERPROFILE%\AppData\Local\Microsoft\NuGet. While I'm sure most experienced Windows developers will agree with me on this it is surprisingly hard to find specific and concrete guidelines on this. The best I have found is section 10 in Certification requirements for Windows Desktop Apps which unfortunately has some typographical errors.
More generally and on Windows, please move all dot-prefixed application data folders and files away from the user profile folder and into the local application data folder where they belong. 🙂
terrajobst
commented
5 years ago I would be so bold as to say that applications should not create their own folder in the user profile folder.
Totally agree. %LocalAppData%\NuGet would be the correct location IMHO.
More generally and on Windows, please move all dot-prefixed application data folders and files away from the user profile folder and into the local application data folder where they belong.
Agree as well.
anangaur
commented
5 years ago @Liversage %LocalAppData%\NuGet aka %USERPROFILE%\AppData\Local\NuGet seems like the right location. We already use this location used for some caching purposes.
alejandro5042
commented
4 years ago A related solution for Windows is a Disk Cleanup Handler, as proposed in #8076 by @miloush.
alejandro5042
commented
4 years ago If we are not adding to the cache, it's certainly not getting bigger, so what's the point in checking it?
Also, when we are adding to the cache, the cost of deleting old nugets may be eclipsed by downloading new nugets (unpredictable over the Internet), unzipping, rewriting package-lock.json files, etc. This is an opportune time to manage our cache.
This strikes the balance @terrajobst mentioned:
[...] there must be a compromise between spamming my drive and impacting my package restore times.
When unpacking a new nuget:
MyNuget.cache-info.json)N GB, delete nugets older than 90 daysWhere the size threshold N can be determined by: max(disk size * 10%, 2 GB)
Yes, this means the cache can grow over past the threshold, but we also don't want to delete nugets that might reasonably still be in use. This is a heuristic meant to help the common case, after all. Users can still clear the cache with:
dotnet nuget locals all --clear
nuget locals all -clear
(etc)Several options on when to add sizes and delete:
If performance is still a factor, we can additionally delete in a background thread:
Some care will need to be done to not kill an in-progress delete. Here are two options:
anangaur
commented
4 years ago From an email discussion regarding this topic:
"...Everyone’s nuget cache and .nuget\packages folder is filling up as we upgrade packages. The only solution we have currently is to delete the whole packages folder and re-download everything, which can be a huge hit on DevOps especially while working remotely. It would great if we had a way to say “delete all the nuget packages except for these packages @ the specified version”.
terrajobst
commented
4 years ago So is this on the books soon? 🙏
anangaur
commented
4 years ago Seems like an issue with most large repos where versions keep on incrementing but the GPF keeps retaining older versions. Looking at ways to ease the pain here.
zivkan
commented
4 years ago We finally had some meetings about this internally.
We discussed some different options, and agreed that the best experience is if we do what some people (me) may consider the most obvious: NuGet should change the filesystem last write time of a single file in each package when it's used in a restore. Tooling to perform the delete will be designed later.
I think the file whose timestamp is updated should either be the .nupkg.metadata, or the packageid.nupkg.sha512 file as both of these files are generated (not extracted from the nupkg). One of these files is used to check if the package already exists in the global packages folder, and this one should be used. I just don't know which one it is.
For packages.config projects, the timestamp should only be updated when the package is copied from the global packages folder to the solution packages folder.
For PackageReference, it's much more complicated:
We have some enterprise and internal customers using some really huge solutions and over the last few months we have made some huge improvements in no-op restore performance. It's unacceptable to regress this perf improvement. Therefore, we need to benchmark whatever implementation/approach we use to ensure the performance is acceptable. If no-op is impacted too much, then we should not update filesystem timestamps on no-op restore, we'll only do it when the no-op fails (work done on the assets file)
My proposal of first implementation to benchmark:
File.Exists(packageMetadataFile) with File.SetLastWriteTime(packageMetadataFile, restoreTime). If SetLastWriteTime does not throw, then the file exists and we avoided needing to call Exists. If SetLastWriteTime throws FileNotFoundException, my hypothosis is that downloading and extracting the package makes the perf cost of thowing and catching an exception insignificant (File.Exists(..) == false doesn't have exception handling perf overheads)
If the perf impact seems too large then we should try:
project.assets.json file may not always be re-written. Apparently there are scenarios where no-op fails, but the assets file is still deemed to be up-to-date and not overwritten. It would be good to update the timestamps in this case since the restore is already going to be quite a bit more expensive than no-op, and hopefully it's not significant.
One complication is that PackageRestore has the concept of a "fallback folder". This is a folder that can be used to get assets, just like the global packages folder, but is read-only. It's often used as an offline source, or a pre-installed source so the packages don't need to be downloaded on first use. These fallback folders are often in read-only parts of the filesystem, so NuGet must not try to update the filesystem timestamp except for the global packages folder (which must have write access, in order to download and extract packages)
We don't have resources to work on this right now, and I hope this is simple enough for someone in the community to make a contribution, so I'm marking the issue as up for grabs. We'll help anyone who would like to take this on, but the first step is to make minimal changes and share a branch, so we can benchmark. Once the concept is validated we'll improve the code to production ready and merge
herebebeasties
commented
3 years ago I've had a quick look at this. Some further info for would-be implementers:
File.Exists check for SHA512 file is done herepublic bool Sha512Exists(string sha512Path, bool updateLastWriteTime) perhaps, and delegate the existing signature through to it passing false for the second parameter.Does that approach sound reasonable @zivkan ? Do we need to keep backwards-compat here for the LocalPackageFileCache ABI?
zivkan
commented
3 years ago @herebebeasties
Do we need to keep backwards-compat here for the LocalPackageFileCache ABI?
yes, but as you mentioned you can create an overload to maintain current behaviour:
public bool Sha512Exists(string sha512Path)
{
return Sha512Exists(sha512Path, isGlobalPackagesFolder: true);
}
public bool Sha512Exists(string sha512Path, bool isGlobalPackagesFolder)
{
// do interesting things here
}Alternatively, since it's not a static method, you could add a class field, and add a constructor with the bool. That way the information doesn't need to be passed around at all the call sites of Sha512Exists, just when the class is instantiated.
Thanks for looking into this, I look forward to learning the results!
herebebeasties
commented
3 years ago Alternatively, since it's not a static method, you could add a class field, and add a constructor with the bool.
Sure. Sounds cleaner. 👍
(We also have some large solutions to test this on with hundreds of projects. Will see if we can get this all wired up and do some performance testing.
As you may well know, File.Exists and FileInfo use the same Win32 API internally (kernel32.dll FindFirstFile which gets a handle and is used to fill a file attributes structure). They are thus essentially identical in performance, so we could grab the FileInfo instead of doing a File.Exists and only update the last write time if the existing date is more than 24h old. Trivial to implement and I cannot imagine a measurable performance difference for the important use-case of repeated builds with no-op restores (e.g. in an IDE). Thanks.
zivkan
commented
3 years ago For what it's worth, I've still never run them myself, but we have some perf scripts, which use the NuGet.Client, OrchardCore and Orleans repos, as well as measuring perf of different scenarios like no-op. It won't handle your new case of last write timestamp > 24h vs < 24 hours. Knowing that difference will be important for this feature, but not the perf scripts more generally.
I don't know the internals of anything under System.IO, sounds like you're more knowledgeable than me. I like your idea about checking the date and updating if it's older than 24 hours. My idea was to replace File.Exists with a try-catch block that calls File.SetLastWriteTimeUtc without checking if the file exists, and catch the FileNotFoundException to represent false. If it doesn't throw, then that means true. But throwing and catching exceptions are slow (although if the package not existing causes a full restore, then the exception will have trivial perf consequences compared to downloading and extracting a nupkg). But if the implementation of SetLastWriteTimeUtc checks if the file exists first, then there's probably no benefit at all to my idea. On the other hand, if updating the timestamp does impact no-op restore too much, then your idea of updating it only once a day might make it acceptable and avoid a more complex implementation of updating the timestamp only on "full" restores. It will probably also need to cache the DateTime.UtcNow value, since getting the time from the RTC isn't free.
zivkan
commented
3 years ago I was too curious, so I wrote a micro-benchmark.
```cs
using System;
using System.IO;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace UpdateTimeStampBenchmark
{
public class Program
{
static void Main(string[] args)
{
BenchmarkRunner.Run
| Method | FileExists | Mean | Error | StdDev | Median | Ratio | RatioSD |
|---|---|---|---|---|---|---|---|
| FileInfoAlwaysUpdate | False | 24.68 us | 0.220 us | 0.206 us | 24.74 us | 0.96 | 0.08 |
| TrySetCatch | False | 63.92 us | 1.765 us | 5.204 us | 60.43 us | 2.50 | 0.23 |
| FileInfoOncePerDay | False | 24.17 us | 0.148 us | 0.132 us | 24.12 us | 0.95 | 0.08 |
| FileInfoOncePerDayWithoutDateTimeCache | False | 23.91 us | 0.079 us | 0.062 us | 23.91 us | 0.94 | 0.08 |
| Baseline | False | 25.72 us | 0.717 us | 2.113 us | 24.93 us | 1.00 | 0.00 |
| FileInfoAlwaysUpdate | True | 164.81 us | 1.627 us | 1.522 us | 164.14 us | 4.22 | 0.06 |
| TrySetCatch | True | 123.02 us | 2.424 us | 2.380 us | 122.76 us | 3.17 | 0.07 |
| FileInfoOncePerDay | True | 40.30 us | 0.297 us | 0.263 us | 40.31 us | 1.03 | 0.02 |
| FileInfoOncePerDayWithoutDateTimeCache | True | 43.44 us | 1.166 us | 3.438 us | 41.46 us | 1.09 | 0.06 |
| Baseline | True | 38.98 us | 0.664 us | 0.518 us | 38.78 us | 1.00 | 0.00 |
My observations:
Having said that, I'm on an SSD. I should spin up a HDD VM and re-run to check the differences, but I would expect the operating system's filesystem cache to make it negligible. edit: Azure's "HDD" VM's disk is faster than my local machine's SATA SSD, at least for this file metadata-only use case. Also, I remembered to test net472 vs net5, and there's no significant differences.
My original idea (always update, use exception when file doesn't exist) is only 100us slower (on a SSD) per file, but since that's about 2x slower than File.Exists, I don't know if the no-op restore time would be impacted too much. However, I'm extremely confident that the 9% slowdown of your once-per-day idea compared to File.Exists will not have a significant impact on no-op restore performance, but we need measurements. I'm optimistic this is going to be feasible.
mfkl
commented
3 years ago I started work in a branch based on you folks recent comments https://github.com/mfkl/NuGet.Client/commit/c0b230e726bd10a8f91e2ccdfc8f75e788131e7e
zivkan
commented
3 years ago @mfkl Thanks for sharing. It looks like a good start to do benchmarking to make sure the no-op perf hit isn't too great. If you use out perf test scripts, they'll need motifications to set the global package folder's timestamps to greater than 24 hours hours between each run.
I'm concerned about the current changes trying to write to file feeds and fallback folders, but let's worry about that after we validate that the performance is going to be acceptable.
mfkl
commented
3 years ago Modified the PS perf script and pasted the command I used here https://github.com/NuGet/NuGet.Client/commit/4811016c565fa928980d0dabfb87ae8fdb5871ea
The test project used for the test is https://github.com/mfkl/test-project
Perf results:
running with a build from branch "cache-expiration" https://github.com/mfkl/NuGet.Client/commits/cache-expiration
NuGet.exe,5.8.0.12833,test-project,2020-10-01T16:36:47.9402121Z,warmup,3.4613896,,False,N/A,1,2.596051,25,13.883122,True,5,2.608838,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12833,test-project,2020-10-01T16:36:47.9402121Z,noop,2.3490578,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12833,test-project,2020-10-01T16:36:47.9402121Z,noop,2.4613719,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12833,test-project,2020-10-01T16:36:47.9402121Z,noop,2.2738489,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4running with a build from branch "dev" b8f519e0a76d99e1b0173841f57b686999c8b61e
NuGet.exe,5.8.0.12836,test-project,2020-10-01T16:51:40.7069823Z,warmup,4.8110862,,False,N/A,1,2.596051,25,13.883122,True,5,2.608838,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12836,test-project,2020-10-01T16:51:40.7069823Z,noop,2.2713532,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12836,test-project,2020-10-01T16:51:40.7069823Z,noop,1.9470795,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.8.0.12836,test-project,2020-10-01T16:51:40.7069823Z,noop,1.7916163,,False,N/A,1,2.596051,25,13.883122,False,5,2.608838,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4@mfkl Thank you again for taking the time to test this. Your single project with a single package test unfortunately does not help us understand the performance impact in scenarios where the perf is more important ("slow", large solutions with many packages).
Plus, assuming the 6th column is the duration, the dev branch commit's mean is 2.00, whereas your branch has a mean of 2.36. That's an 18% decrease in performance. If this is the only evidence we have, we cannot accept this change. However, I'm hopeful that when NuGet is doing more work (more complex package graph to calculate), then the percentage of the total execution time that is impacted by setting the file's timestamp will be much smaller.
mfkl
commented
3 years ago Thanks for your message.
Your single project with a single package test unfortunately does not help us understand the performance impact in scenarios where the perf is more important ("slow", large solutions with many packages).
I hear you. Did another test with Xamarin.Forms.
Xamarin.Forms
running from dev d6dc0502b45cee9857187f26f3b05860e13256d4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:29:40.3733133Z,warmup,61.1637815,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:29:40.3733133Z,noop,6.4101524,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:29:40.3733133Z,noop,6.2865053,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:29:40.3733133Z,noop,6.3167409,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:32:10.4732460Z,warmup,63.0293473,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:32:10.4732460Z,noop,6.6079843,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:32:10.4732460Z,noop,6.5031687,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:32:10.4732460Z,noop,6.5640604,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:34:21.4134477Z,warmup,66.1400479,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:34:21.4134477Z,noop,7.151329,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:34:21.4134477Z,noop,6.3770533,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17621,Xamarin.Forms,2020-10-18T07:34:21.4134477Z,noop,6.6351872,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
running from my branch
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:51:46.6310280Z,warmup,73.6821148,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:51:46.6310280Z,noop,6.6876918,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:51:46.6310280Z,noop,6.8043695,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:51:46.6310280Z,noop,6.715595,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:54:55.0589234Z,warmup,85.0314078,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:54:55.0589234Z,noop,7.1594961,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:54:55.0589234Z,noop,6.7014719,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:54:55.0589234Z,noop,6.6619858,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:57:18.1781234Z,warmup,102.1041198,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451409,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:57:18.1781234Z,noop,6.6467239,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:57:18.1781234Z,noop,6.6830811,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.17625,Xamarin.Forms,2020-10-18T07:57:18.1781234Z,noop,6.8766057,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451409,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4Running the perf test with
.\RunPerformanceTests.ps1 -nugetClientFilePath ..\..\artifacts\NuGet.CommandLine\16.0\bin\Release\net472\NuGet.exe -solutionFilePath ..\..\..\Xamarin.Forms\Xamarin.Forms.sln -resultsFilePath result.txt -skipCleanRestores -skipColdRestores -skipForceRestoresBuilt NuGet with
.\build.ps1 -SkipUnitTest -Configuration ReleaseMy branch: https://github.com/mfkl/NuGet.Client/tree/cache-expiration (rebased on current dev as of this writing).
Running from admin PS shell (non-admin seemed to fail to access some dotnet runtime resources, might have been skewing the results) and Release mode. Multiple successive runs show that there are multiple % differences in the benchmark results for the same build.
zivkan
commented
3 years ago @mfkl sorry I took so long to reply. Thanks for the results! that's a 3.5% perf reduction, but it's once per 24 hours.
Could you try one more time, this remove remove the change to the perf script that sets the last write time stamp to 2 days ago. This lets us compare "inner-build no-op".
With this info, I'll take it to the team's perf champ to see if I can convince him this is a good feature to add.
mfkl
commented
3 years ago Ok, numbers with dev + https://github.com/mfkl/NuGet.Client/commit/bbb3980fa8212485c5752b2c975c85614fe13844 (no script changes). As I previously noted, running it multiple times shows quite some variations FYI.
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:05:10.6177125Z,warmup,82.9721054,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451734,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:05:10.6177125Z,noop,7.2195217,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:05:10.6177125Z,noop,8.3635473,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:05:10.6177125Z,noop,7.1540589,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:07:51.1036780Z,warmup,67.3597338,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451734,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:07:51.1036780Z,noop,6.8763921,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:07:51.1036780Z,noop,6.7952857,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:07:51.1036780Z,noop,6.9678414,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:16:37.3278229Z,warmup,126.5116757,,False,N/A,244,214.766903,6570,919.115725,True,491,212.451734,True,0,0,True,True,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:16:37.3278229Z,noop,6.7467346,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:16:37.3278229Z,noop,6.7844838,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4
NuGet.exe,5.9.0.20781,Xamarin.Forms,2020-10-29T07:16:37.3278229Z,noop,6.8256099,,False,N/A,244,214.766903,6570,919.115725,False,491,212.451734,False,0,0,False,False,Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz,2,4With this info, I'll take it to the team's perf champ to see if I can convince him this is a good feature to add.
Great, thanks.
zivkan
commented
3 years ago Interesting, the restore time when it did not need to modify last modified times are about the same as when it does update the last write time. This doesn't line up with my microbenchmarks where checking the last write time, without updating it, was only very slightly slower than File.Exists, while updating the last write time was much slower (in percentage terms).
All your tests with the modification are consistently slower than the test without your changes, which gives strong evidence that this code change does impact performance, but the results don't have the same trend as the microbenchmarks, so I don't feel like I understand what's really going on here.
I think we need more information. Maybe microbenchmarks of LocalPackageFileCache.Sha512Exists will help, or maybe we need PerfView traces before and after to compare. While I personally think that 300ms on a 6.5s restore (so, just under 5%) seems ok, I'm not involved in the perf meetings with partners and executives, so I don't know if this will pass.
mfkl
commented
3 years ago While I personally think that 300ms on a 6.5s restore (so, just under 5%) seems ok,
Running these benchmarks on both dev and my branch produced quite some different results on many successive runs. My opinion is that if you are going to be looking at fractions of seconds diffs, then the current perf script (e.g. starting an exe manually) won't provide reliable perf results.
Maybe microbenchmarks of LocalPackageFileCache.Sha512Exists will help
Yes, I'll try to put that together and let you know.
zivkan
commented
3 years ago Running these benchmarks on both dev and my branch produced quite some different results on many successive runs. My opinion is that if you are going to be looking at fractions of seconds diffs, then the current perf script (e.g. starting an exe manually) won't provide reliable perf results.
Perhaps running the script more times, or changing the script to execute more runs, will increase our confidence. This is exactly what BenchmarkDotNet does when it detects what it's running has high standard deviation. Low standard deviation benchmarks run 15 times, high standard deviation benchmarks get up to 100 runs.
The randomness/variability of starting an exe is the same between the dev and feature branch builds of nuget.exe, so larger number of runs will smooth it out. I didn't do a statistical analysis to check if the mean of the before-vs-after runs are within the "margin of error", but it certainly looks like your feature branch is systematically slower than the dev branch. Unless someone can point out a reason why this method of testing is systematically unfair to one nuget.exe and not the other, then I believe that a larger number of tests is sufficient. Next time I can also calculate standard deviation to check if the mean values of the two nuget.exes are within "margin of error". That's the limit of my statistical skills, however.
In another issue you mentioned that your feature branch is changing the timestamp of files not only in the global packages folder, but also read-only fallback folders. This is not only a bug, but also means that your feature branch is doing more work than is necessary, and therefore we could improve performance by fixing the bug. An alternative is to ensure that benchmarks are using a nuget.config that clears fallback folders (the example doesn't show how to clear, but it's the same as package sources, use <clear/>), so only the global packages folder is used. Once we determine that the feature has acceptable perf, we can spend time on fixing the bug.
Yes, I'll try to put that together and let you know.
For what it's worth, I believe that in December I'll have capacity to work on this for a few days. I don't want to "steal credit" for your work, so I can find other work to do if you'd like to drive this to completion yourself and have a PR/commit with only your name. Otherwise I can copy your branch, so your name is in the commits and try to complete it myself. Having said that, December is a month away, so if we don't encounter too many more problems you might be able to complete it before then anyway.
mfkl
commented
3 years ago Perhaps running the script more times
I have ran the script dozen of times on either branches and get wildly different results. If my CPU is already at 20% usage from other apps just before running the script, perf numbers go x2 easily.
In another issue you mentioned that your feature branch is changing the timestamp of files not only in the global packages folder, but also read-only fallback folders.
Right, I've made a bad hack to fix this temporarily https://github.com/mfkl/NuGet.Client/commit/5d2c27abe0a51c784216fec03ab77568ac85e1e2. This change won't fix the result deviations though, only using benchmarkdotnet would I'm afraid.
I don't want to "steal credit" for your work
Thanks, I appreciate your concern, but I don't really care about credits for this. As long as Software gets better, I'm happy :-)
Danielku15
commented
3 years ago I stumbled over this issue in relation to another topic and I would like to give just a small input for consideration on clean up policies. I wasn't able to quickly grasp all details of the already long ongoing discussion so I dare to just drop my input in the hope it fits into the current plans.
NuGet supports floating versions which can be quite useful if you have continuous deployment of your packages (in our case we have still old fashioned nightlies for some package deploys). If you go for a semver like 1.0.0-alpha.### and consume the package via 1.0.0-alpha.* you will get continuously new packages if they are available. This also means after a while you end up with a lot of pre-release versions which might not be actually relevant anymore.
It would be great if the cleanup policy could be shorter by default for such pre-release packages. I would expect in most of the cases you would always only need the maybe one or two latest pre-release version of one tag for a particular library version in the local cache.
eatdrinksleepcode
commented
2 years ago Needed to manually delete some large older packages from my cache today to make room on my disk, and found this issue. As Immo said, this is a leak, and as such I am disappointed that it hasn't been prioritized more highly.
There are two potential problems I can see with the spec as proposed:
/cc @zivkan
zivkan
commented
2 years ago Thanks to @mfkl for implementing the "core" part that will update the last access time of one file in each package on restore: https://github.com/NuGet/NuGet.Client/pull/4222
I'm guessing it's going to ship in .NET SDK 6.0.300 and VS 17.2.
Once it's shipped, and we start using it for all our solutions, then we'll be able to use tools like https://github.com/chgill-MSFT/NuGetCleaner
C-Wal
commented
1 year ago Personally I'd prefer to have an option that kept the last n number of versions downloaded of each package instead of simple date-based expiry.
jaredthirsk
commented
8 months ago If you're lazy like me, you might want to use @dotMorten's code through this
dotnetglobal tool: https://github.com/terrajobst/dotnet-nuget-gc
Done! Deleted 62,359 MB.
Wow, that's a lot.
edit by @zivkan: a first step has been implemented, allowing package "last used in restore" date to be updated: https://github.com/NuGet/NuGet.Client/pull/4222
original post below:
The user local NuGet cache has a tendency to grow unconditionally. Which results in funny tweets like this from @davidfowl:
Can we track when an item was last hit by package restore and apply some expiration policy? I don't have good data, but I bet a good chunk of the bloat aren't individual packages with a single version but a small number of packages with many versions. If were to just delete older versions that weren't needed for restore for a while, we're probably getting to something saner.
Thoughts?