-
Notifications
You must be signed in to change notification settings - Fork 391
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
Fast up to date check for .NET Core projects #2241
Fast up to date check for .NET Core projects #2241
Conversation
@dotnet/project-system @davkean @jasonmalinowski |
@@ -29,6 +29,13 @@ | |||
<ItemGroup> | |||
<Folder Include="Properties\" /> | |||
</ItemGroup> | |||
<ItemGroup> | |||
<PackageReference Include="Microsoft.Tpl.Dataflow" Version="4.5.24" /> |
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.
Should these just be added here? Also shouldn't some of these already be pulled in MS.VS.ProjcetSystem should come in because we include the MS.VS.ProjectSystem.SDK package which should pull in Tpl.Dataflow, composition and threading. Do you just need to update the version there maybe? #Resolved
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.
In fact I think you don't need any of these packages. They should all come in transitively from the Projectsystem.SDk package. #Resolved
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 think the package references were a transitory problem I had. There was an issue with an older version of the project system getting pulled in and this forced it to the right version. I removed the references and everything seems OK so the problem must have passed.
In reply to: 117537469 [](ancestors = 117537469)
[Guid("9B164E40-C3A2-4363-9BC5-EB4039DEF653")] | ||
private class SVsSettingsPersistenceManager { } | ||
|
||
private const string FastUpToDateDisabledSettingKey = "NETCoreProjectSystem\\FastUpToDateCheckDisabled"; |
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.
Maybe called the key ManagedProjectSystem instead of NETCoreProjectSystem to future-proof #Resolved
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.
use string literals to avoid double backslash #Resolved
#if DEBUG | ||
return true; | ||
#else | ||
return _settingsManager?.GetValueOrDefault(FastUpToDateEnabledSettingKey, false) ?? false |
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.
Should be !FastUpToDateDisabledSettingKey #Resolved
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.
Actually that sounds wrong too -- should be OutputPaneEnabledSettingKey? We don't want to log any time fast uptodate is enabled right? #Resolved
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.
{ | ||
var project = await access.GetProjectAsync(_configuredProject, cancellationToken); | ||
|
||
void Log(string traceMessage) |
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.
Local functions! nice :) #Resolved
|
||
var timestampCache = _fileTimestampCache != null ? _fileTimestampCache.Value.TimestampCache : new Dictionary<string, DateTime>(StringComparer.OrdinalIgnoreCase); | ||
|
||
DateTime GetLatestTimestamp(List<string> paths) |
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.
Not sure how I feel about larger local functions - maybe better as private static methods - otherwise it gets in the way of reading the code - make this method look huge. #Resolved
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.
+1
I agree, I think those methods which do not depend on any state inside this function, should be the member of this class, instead of local functions. That is the same for the Log method, it only uses the _projectLogger, which is a member of the class, not anything local to this method. That is especially true when those methods is valid and has its own logic meaning when it lives outside of this method.
I think they should be moved out to be other private member method of this class. I totally agree with @srivatsn that this makes this method huge and hard to read. #Resolved
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.
Jacob also has a good point: the entire class is to implement this method, so there is no reason to implement a single method, and put all help function inside it. #Resolved
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.
Like all monsters, it started small and just kind of took on a life of its own, moved it out.
In reply to: 117559029 [](ancestors = 117559029)
return false; | ||
} | ||
|
||
if (!string.IsNullOrEmpty(project.GetPropertyValue(DisableFastUpToDateCheckProperty))) |
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.
Normally I'd expect that we'd add a rule for this property and get it from projectProperties.GetConfigurationGeneral.GetDisableFastUpToDateCheckProperty() - not sure if there's an advantage to going through that route - @lifengl? #Resolved
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.
The rule provide an abstract layer, and also provide a way to access it in a data flow or through an automation layer. this one here might be fine. @jviau ? #Resolved
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.
Using the project properties has a some advantages:
- You can change the backing source of the property in the future without have to update code (maybe you want this to go into the .user file eventually?)
- Gives you clear choices between evaluated value and unevaluated value.
- Removes the need for your own project lock. Doesn't matter here as this is already in a lock. #Resolved
var itemTypes = projectItemSchemaValue | ||
.GetKnownItemTypes() | ||
.Select(name => projectItemSchemaValue.GetItemType(name)) | ||
.Where(item => item != null && item.UpToDateCheckInput && !string.Equals(item.Name, "None", StringComparison.OrdinalIgnoreCase)); |
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.
Some of these special-cases (like None, ExcludedFromBuild etc. are interspersed in a giant method - can we factor some of this out with fields like excludedItems, excludedMetadata etc. #Resolved
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.
As Sri stated, it would be nice if you could split these huge methods into individual methods. Will be much easier to read. #Resolved
[Guid("9B164E40-C3A2-4363-9BC5-EB4039DEF653")] | ||
private class SVsSettingsPersistenceManager { } | ||
|
||
private const string FastUpToDateDisabledSettingKey = "NETCoreProjectSystem\\FastUpToDateCheckDisabled"; |
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.
use string literals to avoid double backslash #Resolved
Requires.NotNull(serviceProvider, nameof(serviceProvider)); | ||
|
||
_environment = environment; | ||
_settingsManager = (ISettingsManager)serviceProvider.GetService(typeof(SVsSettingsPersistenceManager)); |
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.
Calling GetService
in the constructor will result in deadlock since we this call needs to be on the UIThread and the object initialization will be on background thread. We have been hit by this issue so many times. The best practice would be to call GetService
when used. You can check the usage in the rest of the code as well. #Resolved
return false; | ||
} | ||
|
||
using (var access = await _projectLockService.ReadLockAsync()) |
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.
pass the cancellationToken to the REadLockAsync as well? #Resolved
{ | ||
if (File.Exists(path)) | ||
{ | ||
time = File.GetLastWriteTimeUtc(path); |
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.
Use FileInfo, which will reduce two IO class (Exists/GetLatestWriteTimeUtc) into one.
var inputs = new List<string>(); | ||
|
||
// add the project file | ||
if (!string.IsNullOrEmpty(project.FullPath)) |
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.
use the UnconfiguredProject.FullPath instead of using the one in msbuild. #Resolved
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.
Thanks, Lifeng. This is what the VC fast up to date check is doing, so should we let them know as well?
In reply to: 117560175 [](ancestors = 117560175)
{ | ||
Log($"Adding input project import {import}."); | ||
} | ||
|
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 code actually run through the LinQ expression twice. You should just do:
var imports = .....ToList(), and use the final list, instead of executing LinQ path twice.
} | ||
} | ||
|
||
async Task CheckReferences<TUnresolvedReference, TResolvedReference>(string name, IResolvableReferencesService<TUnresolvedReference, TResolvedReference> service) |
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.
add Async
suffix. #Resolved
var itemTypes = projectItemSchemaValue | ||
.GetKnownItemTypes() | ||
.Select(name => projectItemSchemaValue.GetItemType(name)) | ||
.Where(item => item != null && item.UpToDateCheckInput && !string.Equals(item.Name, "None", StringComparison.OrdinalIgnoreCase)); |
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.
at least those strings like 'None' should be pulled out be constants. Also, why None is treated that differently here? should the information to be put in the item schema rule (item.UpToDateCheckInput) instead? #Resolved
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.
|
||
// add all project items (generally items seen in solution explorer) that are not excluded from UpToDate check | ||
// Skip items that are marked as excluded from build. | ||
var projectItemSchemaValue = (await _projectItemsSchema.GetSchemaAsync(cancellationToken)).Value; |
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.
Also, instead of a huge function like this, those item schema work might be pulled out to be private methods.
Log($"Checking known item type '{itemType.Name}'."); | ||
|
||
var items = project.GetItems(itemType.Name) | ||
.Where(item => !string.Equals(item.GetMetadataValue("ExcludedFromBuild"), "true", StringComparison.OrdinalIgnoreCase)) |
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.
pull out constant strings #Resolved
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.
BTW, I think we should call source items service here, and map ExcludedFromBuild through the project item rules. #Resolved
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.
Actually, I realized that ExcludedFromBuild came from the VC fast up to date check and that's a VC-only property. I'll remove it.
In reply to: 117564679 [](ancestors = 117564679)
// Skip items that are marked as excluded from build. | ||
var projectItemSchemaValue = (await _projectItemsSchema.GetSchemaAsync(cancellationToken)).Value; | ||
var itemTypes = projectItemSchemaValue | ||
.GetKnownItemTypes() |
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 think the code should use the sources items service to get source items, instead of doing this. The problem is that known item types may have items types which are not file items (like web references). The source item service handles it. #Resolved
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.
OK, will switch over to use the source items service.
In reply to: 117564209 [](ancestors = 117564209)
{ | ||
[AppliesTo(ProjectCapability.CSharpOrVisualBasic)] | ||
[Export(typeof(IBuildUpToDateCheckProvider))] | ||
internal class BuildUpToDateCheck : IBuildUpToDateCheckProvider |
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.
Have you considered before or after draining of critical tasks? By default it is after drain critical tasks. Is there some logic that can be brought into an update to date checker that runs before draining? For example if a nuget restore is running you can immediately return false
as that restore will most likely affect the build.
The metadata you want is this:
[ExportMetadata("BeforeDrainCriticalTasks", true)]
. Default value is false
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.
Also, I would consider either splitting up into multiple IBuildUpToDateCheckProvider
that cover smaller checks. This implementation is trying to be a catch-all check and may become difficult to maintain in the future.
If you don't want multiple implementations, then I would try to containerize each individual check performed here into helper classes, or at least individual functions.
} | ||
} | ||
|
||
async Task CheckReferences<TUnresolvedReference, TResolvedReference>(string name, IResolvableReferencesService<TUnresolvedReference, TResolvedReference> service) |
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.
move it to a member function of the class. #Resolved
/// <exception cref="FormatException"> | ||
/// The format specification in <paramref name="format"/> is invalid. | ||
/// </exception> | ||
void WriteLine(string format, params object[] arguments); | ||
} |
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.
Context on why this was removed?
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 see why you removed this - you used interpolated strings, this is very deliberate that I did not use them, they allocate on every usage, even when logging is turned off.
@@ -58,7 +58,7 @@ | |||
<NameValuePair Name="DefaultMetadata_Generator" Value="ResXFileCodeGenerator" /> | |||
</ContentType> | |||
|
|||
<ItemType Name="None" DisplayName="None"/> | |||
<ItemType Name="None" DisplayName="None" UpToDateCheckInput="False"/> |
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.
Why items and not Context/EmbeddedResource?
[Guid("9B164E40-C3A2-4363-9BC5-EB4039DEF653")] | ||
private class SVsSettingsPersistenceManager { } | ||
|
||
private const string FastUpToDateDisabledSettingKey = @"ManagedProjectSystem\FastUpToDateCheckDisabled"; |
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.
We should be respecting existing property around this.
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.
@davkean What existing property? I'm happy to not add something new, but I didn't see an existing option that managed this.
private readonly IEnvironmentHelper _environment; | ||
|
||
[ImportingConstructor] | ||
public ProjectSystemOptions(IEnvironmentHelper environment, [Import(typeof(SVsServiceProvider))]IServiceProvider serviceProvider) |
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.
Please use IVsOptionalService<ISettingsManager, SVsSettingsPersistenceManager> instead of IServiceProvider.
public async Task<bool> GetIsProjectOutputPaneEnabledAsync() | ||
{ | ||
await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync(); | ||
return SettingsManager?.GetValueOrDefault(OutputPaneEnabledSettingKey, false) ?? false |
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 used to be enabled in debug builds, it's not anymore.
|
||
namespace Microsoft.VisualStudio.ProjectSystem | ||
{ | ||
[AppliesTo(ProjectCapability.CSharpOrVisualBasic)] |
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.
Why not F#?
@@ -55,6 +55,11 @@ | |||
<DataSource Persistence="ProjectFileWithInterception" PersistedName="TargetFrameworkMonikers" HasConfigurationCondition="False" SourceOfDefaultValue="AfterContext"/> | |||
</StringProperty.DataSource> | |||
</StringProperty> | |||
<BoolProperty Name="DisableFastUpToDateCheck" DisplayName="Disable Fast Up To Date Check"> | |||
<BoolProperty.DataSource> | |||
<DataSource Persistence="ProjectFile" PersistedName="DisableFastUpToDateCheck" HasConfigurationCondition="False" SourceOfDefaultValue="AfterContext" /> |
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.
Can we reverse the logic of this property FastUpToDateCheckEnabled? Positive properties are so much easier to parse in your head, then negative. See Raymond Chen's rant on this.
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.
Looks like this is existing name, be sure to check out CVsProjBuildableProjectCfg::IsFastUpToDateCheckEnabled for the legacy project system.
var inputs = new List<string>(); | ||
|
||
// add the project file | ||
if (!string.IsNullOrEmpty(_configuredProject.UnconfiguredProject.FullPath)) |
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 isn't enough, what if the user file change or if targets change? I think either we should at least respect <MSBuildAllProjects>
.
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.
Originally, the code did add the imports to the project, but another comment stated that it wasn't necessary. Upon further investigation, it looks like it is, so I'm adding the code back. If we do that, do we still need to look at MSBuildAllProjects?
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 targets should be enough, thinking about it MSBuildAllProjects
is only needed for <Target>
because they can't say all targets are my input.
|
||
foreach (var item in await _projectItemProvider.GetItemsAsync()) | ||
{ | ||
var itemType = projectItemSchemaValue.GetItemType(item); |
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.
What if None
or Content
items are marked as AlwaysCopy?
_fileTimestampCache = fileTimestampCache; | ||
} | ||
|
||
private void Log(string traceMessage) => _projectLogger.WriteLine($"FastUpToDate: {traceMessage}"); |
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 think logging to the project output window is the wrong category - a common complaint is that user's aren't aware of why their project built. I think we should always log to the Build output window why we starting building.
IProjectLogger projectLogger, | ||
ProjectProperties projectProperties, | ||
[Import(ExportContractNames.ProjectItemProviders.SourceFiles)] IProjectItemProvider projectItemProvider, | ||
[Import(AllowDefault = true)] IProjectItemSchemaService projectItemSchema, |
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.
When would these not exist?
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.
Not sure when they wouldn't exist, I think this was in the skeleton I adopted from VC. As far as I can see, they should always exist, so will remove the AllowDefault.
// Extremely naive implementation of a Windows Pane logger - the assumption here is that text is rarely written, | ||
// so transitions to the UI thread are uncommon and are fire and forget. If we start writing to this a lot (such | ||
// as via build), then we'll need to implement a better queueing mechanism. | ||
_threadingService.Fork(async () => { | ||
if (await _options.GetIsProjectOutputPaneEnabledAsync()) |
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.
It was very deliberate that we did close to zero work when the logging is disabled. This and interpolated strings change has changed that - in the language service we log a lot - I want zero impact on that when logging is off. #Resolved
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.
@davkean Sorry, I should have made it clear that I just hacked the logging to get it working and was going to spend more time on it once other things were dealt with. I thought it would be nice to use the window you're adding, but there are threading issues since the up to date check isn't running on the UI thread. I tried to hack a solution, but it's probably better that I just use the logger I'm given (which goes to the build output window). #Resolved
{ | ||
if (!timestampCache.TryGetValue(path, out var time)) | ||
{ | ||
if (File.Exists(path)) |
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.
There's a race here between File.Exists and File.GetLastWriteTimeUtc, instead you should handle when GetLastWriteTimeUtc returns a value indicating that the file is deleted.
if (File.Exists(path)) | ||
{ | ||
time = File.GetLastWriteTimeUtc(path); | ||
timestampCache[path] = time; |
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.
What about duplicate items? This should be fine.
} | ||
else | ||
{ | ||
Log($"Path '{path}' did not exist."); |
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.
So if file is deleted from disk in a *.cs situation, shouldn't that result in a build?
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.
Yes, I thought about that over the weekend. If an input or output file is missing, we should fail the up to date check.
{ | ||
foreach (var resolvedReference in await service.GetResolvedReferencesAsync()) | ||
{ | ||
var fullPath = await resolvedReference.GetFullPathAsync(); |
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.
Does this result in a build? I think it does.
Looks like legacy project system disables fast-up-date for a lots of scenarios, see: CLangUpToDateCheck::DoCheck. |
Where do analyzers, additional files, etc come into play here? |
@panopticoncentral Turn on design-time builds to see the builds that occurring under the covers when this up-to-date check occurs. |
Since there has been significant revision, I'm going to close this PR and start a new one. |
Customer scenario
When the user requests a build in VS, CPS will call the project system to see if we believe it can skip calling MSBuild. This is faster than running MSBuild and having it determine that everything is up to date.
Bugs this fixes:
#62
Workarounds, if any
This is a performance optimization, it is not required for functionality, just speed.
Risk
Fast up to date checks carry a level of risk because if the check is wrong, we will either build when we didn't need to, or won't build when we need to. The former situation isn't any worse than what we have now. The latter situation is bad but can be mitigated in several ways:
DisableFastUpToDateCheck
property that will disable the check for that projectUpToDateCheckInput
andUpToDateCheckOutput
items to their project to work around missing checks.This PR also adds logging to a "Project" output window that should help easily diagnose where problems are.
Performance impact
Build performance should improve, especially for solutions with many .NET Core projects.
Is this a regression from a previous update?
No