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

Add options to configure background analysis in the IDE #38803

Closed
wants to merge 2 commits into from

Conversation

mavasani
Copy link
Contributor

  1. Option to enable power save mode: Fixes Restrict background analysis scope to reduce CPU consumption in live analysis #38429. This option turns off all the background analysis and also turns off remote host (RoslynCodeAnalysis service hub process) for C# and VB projects to reduce memory consumption in this mode. This option turns off most of the background analyses that are executed by the solution crawler. We still execute the bare minimum analysis for a reasonable editing experience: compiler analyzer to get compiler warnings/errors and also execute the add usings and remove unncessary usings analyzers. We can tune this required analysis set in future, as required.

  2. Option to disable live analyzer execution: There have been lot of requests in past to give users a single knob to turn on/off execution of all Roslyn analyzers in the IDE (third party analyzers + IDE code style analyzers which are enabled by default).

I have also hooked up the low virtual memory listener which detects low VM and used to turn off just the full solution analysis to now instead turn on a forced power save mode for the current VS session (which also turns off full solution analysis, along with other background analyses).

TODOs:

  1. More manual testing
  2. Get design consensus on the preferred option(s):
    1. 2 options (current PR): Options to configure Power save mode and Analyzer execution
    2. Single option to configure Power save mode only
    3. Multiple options: One top level option to configure Power save mode, with lot of individual options to control individual background analyses. This is less preferable due to overload of options and information for users.

1. Option to enable power save mode: Fixes dotnet#38429. This option turns off all the background analysis and also turns off remote host (RoslynCodeAnalysis service hub process) for C# and VB projects to reduce memory consumption in this mode. This option turns off most of the background analyses that are executed by the solution crawler. We still execute the bare minimum analysis for a reasonable editing experience: compiler analyzer to get compiler warnings/errors and also execute the add usings and remove unncessary usings analyzers. We can tune this required analysis set in future, as required.

2. Option to disable live analyzer execution: There have been lot of requests in past to give users a single knob to turn on/off execution of all Roslyn analyzers in the IDE (third party analyzers + IDE code style analyzers which are enabled by default).

I have also hooked up the low virtual memory listener which detects low VM and used to turn off just the full solution analysis to now instead turn on a forced power save mode for the current VS session (which also turns off full solution analysis, along with other background analyses).
@mavasani mavasani added this to the 16.4 milestone Sep 23, 2019
@mavasani mavasani requested review from heejaechang, sharwell, a team and genlu September 23, 2019 21:00
@CyrusNajmabadi
Copy link
Member

This option turns off all the background analysis

So we don't get squiggles for the current file someone is in for anything? (i.e. user errors or analyzers)?

and also turns off remote host (RoslynCodeAnalysis service hub process) for C# and VB projects to reduce memory consumption in this mode.

What happens with something like FindRefs/NavTo/etc. note that these tend to perform much worse in VS and cause much more overhead and pressure on the GC, causing more cost to the system.

Also, i'm not sure if we removed it, but we changed certain algorithms to be more parallel given that we thought they were now always OOP. THat tends to produce way more garbage which is significant when it's in the same proc as the VM for al the other managed allocs.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Sep 23, 2019

Wat:

image

Were all the loc changes intentional?

@@ -51,7 +52,8 @@ public async Task AnalyzeSyntaxAsync(Document document, InvocationReasons reason
// but, can be called concurrently for different documents in future if we choose to.
Contract.ThrowIfFalse(document.IsFromPrimaryBranch());

if (!document.Project.Solution.Options.GetOption(InternalFeatureOnOffOptions.TodoComments))
if (!document.Project.Solution.Options.GetOption(InternalFeatureOnOffOptions.TodoComments) ||
ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(document.Project))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrmm... so what's the experience like with this sort of thing. Consider if the user has existing todo comments up. Then they enter powersaving mode. T hen they remove one. The todo comment just stays up right? This will appear as if things are broken.

Can we instead return an empty list and yellow-bar the todo window (or place a dummy item into it) saying that todo items are not being reported because of power-saving?

@@ -383,7 +383,8 @@ private static bool FullAnalysisEnabled(Project project, bool forceAnalyzerRun)
}

if (!ServiceFeatureOnOffOptions.IsClosedFileDiagnosticsEnabled(project) ||
!project.Solution.Options.GetOption(RuntimeOptions.FullSolutionAnalysis))
!project.Solution.Options.GetOption(RuntimeOptions.FullSolutionAnalysis) ||
ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(project))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point, i basically don't understand what controls FSA.

@mavasani
Copy link
Contributor Author

mavasani commented Sep 23, 2019

So we don't get squiggles for the current file someone is in for anything? (i.e. user errors or analyzers)?

W.r.t analyzers, we will only execute the core compiler analyzer that reports compiler diagnostics + important IDE analyzers (such as add using, remove unnecessary using).

What happens with something like FindRefs/NavTo/etc. note that these tend to perform much worse in VS and cause much more overhead and pressure on the GC, causing more cost to the system.

I believe the idea here was that most of the features become pay for play, where they might actually turn out to perform worse then in non-power save mode, but they will all be driven by explicit user action. Again, note that we are just brainstorming on initial set of defaults to fit into this power save philosophy to not use any resources until the users request. We might find out that it is better to keep the OOP process still running for majority of scenarios, and then revert the added code that is turning off remote host.

@@ -319,7 +320,8 @@ private bool SupportAnalysis(Project project)
return false;
}

return RemoteFeatureOptions.ShouldComputeIndex(project.Solution.Workspace);
return RemoteFeatureOptions.ShouldComputeIndex(project.Solution.Workspace) &&
!ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(project);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we want this. Instead what i think we shoudl do is not precalculate things when we're in powersaving mode. this looks like this would break all symbol-tree-info features, even if the user explicitly asks to use it.

instead, i think it makes more sense to make sure those features don't cause undue power usage unless explicitly requested by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so basically just guard the incremental analyzer in this file for this option, but not the entire SupportsAnalysis functionality. That seems reasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! :D

@@ -23,9 +23,10 @@ internal abstract class AbstractRemoveUnnecessaryImportsDiagnosticAnalyzer
// The NotConfigurable custom tag ensures that user can't turn this diagnostic into a warning / error via
// ruleset editor or solution explorer. Setting messageFormat to empty string ensures that we won't display
// this diagnostic in the preview pane header.
// Setting category to "Compiler" ensures we show the unnecessary usings even in power save mode.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this is such a hack. why is remove-imports in this category, but other fixes would not be? somthing is fishy here.

How about: in power saving, we only run for the visible file. Other files only get fixed up when opened or get focus?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about: in power saving, we only run for the visible file. Other files only get fixed up when opened or get focus?

Only run what? We want to explicit turn off any analyzers that are not required for minimal acceptable IDE experience. We can possible add a new custom tag for such analyzers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only run what?

Only run analyzers. However, we don't run them for any other files. That will still help out with power savings, while not diminishing the experience for hte actively edited file. I think users would understand and be ok with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could also be potentially expensive, if you have an expensive analyzer running. I think our preference was to only run extremely important analyzers just for active file. It is fine to create a new mechanism to identify those, but we don't want to run other analyzers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. I think this is a reasonable place to start.

/// </summary>
public static readonly Option<bool> PowerSaveMode = new Option<bool>(
nameof(ServiceFeatureOnOffOptions), nameof(PowerSaveMode), defaultValue: false,
storageLocations: new RoamingProfileStorageLocation($"Options.PowerSaveMode"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you clarify the option. it is set dynamically when the system goes into low-power mode, or is it set by the user to say "hey, when i go into low power, then don't do as much work".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is only controlled by the end user. The dynamic low VM mode sets a separate boolean flag (below in this file) that turns on power save mode just for current VS session (not serialized/saved anywhere).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so... what i find weird is this. We're driving this by low VM. But we're not driving by low-battery (where i would want the power savings)? Wouldn't the latter make more sense.

Also, by disabling OOP, don't you hurt VM by packing more into the single process? Or is this machine-wide VM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so... what i find weird is this. We're driving this by low VM. But we're not driving by low-battery (where i would want the power savings)? Wouldn't the latter make more sense.

We can also do that - I just reused the existing low VM listener that we already have that handles scenarios where we need to suspend features to improve IDE experience. We can also do a similar handling for low battery scenario.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nifty!

@@ -98,6 +99,11 @@ private void Rebuild(Solution solution, ProjectId initialProject = null)
// build the current compilations without rebuilding the entire DeclarationTable
CancelBuild(releasePreviousCompilations: false);

if (ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(solution.Options))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably not a good idea. all it means is more contention for the N features that wake up and need a compilation. It's going to happen on every keystroke, so this feature just helps ensure the compilation is there and ready for all of them.

i think we should delay work in the cases where we know it's only triggered by uesr action (i.e. find-refs/nav-to/other-docs). But having the data ready for the features that will still run is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm @heejaechang @sharwell what do you think? One of the things we discussed was that none of the background analysis should pre-emptively realize any compilations for power save mode. That would be invalidated if we kept the background compiler still running.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, i did a test at one point in time, and at least for syntaxtrees, every keystroke ends up needing the tree.

I'm not sure where we landed with compilations, but i thought the same was true. It would be really interested to measure this and see two things:

  1. what percentage of compilations less does this cause?
  2. does this actually help?

'2' is relavant. I remember us trying to delay work in the past. However, it turned out to be more costly because it was cheaper and simpler for the compiler to make N tiny changes than one large change. For example, incremental compilation does a ton better here. And if incremental compilation is better, then we do a better job saying "we can reuse the old compilation and get a speculative semantic model".

Note: this only relates to BackgroundCompiler.

if (oldProject.SupportsCompilation && !object.Equals(oldProject.ParseOptions, newProject.ParseOptions))
if (!ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(newProject) &&
oldProject.SupportsCompilation &&
!object.Equals(oldProject.ParseOptions, newProject.ParseOptions))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. need to update this doc.
  2. this behavior seems undesirable... if the project changed and the parse options were updated, it's really important to update our syntax trees. otherwise, all sorts of debu/release stuff will be incorrect.

This is another case of the user taking an explicit action, so i think it's ok to do work in response to that. i think we want to avoid the costly stuff the user didn't explicitly ask to have happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi Will this feature cause the compilation to be realized? Note that I still need to test the effects of this change thoroughly, but presume you already know.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i retract '2'. We should still get up to date synta xtree, we just won't proactively make it happen.

So i'm ok with this change, but i also think it's marginal. Anything that needs the semantic model (which is like everything) will need up to date trees, so they're going to force those trees to reparse anyways.

--

Another way of thinking aobut it: classification is fast and causes semantics (i.e. the compilation) to be computed for the project on each keystroke. If you work causes us to avoid computation that classification would cause anyways, it's not really doing much afaict.

on the other hand, having us not keep our indices up to date in the background is relevant as those may want entire symbol graphs for a project.

@@ -15,6 +15,10 @@
<GroupBox x:Uid="AnalysisGroupBox"
Header="{x:Static local:AdvancedOptionPageStrings.Option_Analysis}">
<StackPanel>
<CheckBox x:Name="Enable_power_save_mode"
Content="{x:Static local:AdvancedOptionPageStrings.Option_Enable_power_save_mode}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how would a user figure out what this means? My view on power saving is not "we are doing less". Rather, it's "we do less work when the machine doesn't have a lot of CPU avaliable". i.e. it's "battery saving mode". This is how phones work for example. When you go into battery saving mode, htey do less things automatically, but you can still do anything you want explicitly. I think we should model after that.

<CheckBox x:Name="Enable_power_save_mode"
Content="{x:Static local:AdvancedOptionPageStrings.Option_Enable_power_save_mode}" />
<CheckBox x:Name="Disable_analyzers"
Content="{x:Static local:AdvancedOptionPageStrings.Option_Disable_analyzers}" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the user doesn't want analyzers running, i think we should be true to that. that means that IDE features that are analyzers (including remove unused usings) shouldn't run. You just get compiler errors.

@@ -75,7 +76,8 @@ public async Task AnalyzeDocumentAsync(Document document, SyntaxNode bodyOpt, In

cancellationToken.ThrowIfCancellationRequested();

if (!document.Project.Solution.Workspace.Options.GetOption(InternalFeatureOnOffOptions.DesignerAttributes))
if (!document.Project.Solution.Workspace.Options.GetOption(InternalFeatureOnOffOptions.DesignerAttributes) ||
ServiceFeatureOnOffOptions.IsPowerSaveModeEnabled(document.Project))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens to the user then? they're just SOL? they want something to be designable, and it just doesn't work? How about we still at least analyze the file whne open/edited. we just don't proactively scan them all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tagging @sharwell @heejaechang for this too. We felt it would be acceptable in power save mode for designers to not open automatically, and the code view is the default. I think Heejae did suggest that we can do what you recommended - just scan attributes in current file. I should be able to make that change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think a bit of a problem is that it's really going to be difficult for users to relate what they perceive as "broken behavior" to this option.

Consider a totally realistic scenario:

  1. i see this option and go "oh great! i've wanted VS to do less work".
  2. i turn it on and really like it. i use vs for a couple fo weeks happily.
  3. i then get around to needing to use the designer. it totally doesn't work because the type doesn't seem designable. I have no idea waht's going on and i think VS is totally broken.
  4. i basically can't "kick" vs out of this state. my only option (which i have to discover as well) is to go turn the option off.

THe problem here is that we've disconnected the option with the impact it may have on so many disparate parts of the UI. Furthermore, this is no longer "pay for play". there's no way for me to state that i'm willing to "pay".

In a case like designers, we could make you "pay" by having to at least open or edit the doc. That would at least be a way to kick things out of the state its in.

For something like todo: we could make it have something in the todo UI to explain why it's not working.

etc. etc.

--

In other words, i strongly support this feature. But basically each feature/module that is updated has to think through the user scenario around it and what's going to be an acceptable model for that interaction. I think this will fall into a few general buckets. But once we identify them, we're golden.

Copy link
Member

@sharwell sharwell Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi In power save mode, forms open in code view by default since it's more efficient. Opening designer view will require the user to explicitly invoke the "View Designer" command, which we are expecting to work properly for shipping this feature.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you click 'view designer' if the DesignerAttributer has not been detected/reported for that file?

If that's the case, I'm ok with this. It's just not something i'm familiar with the workings of. A screenshot of this in action @mavasani would be helpful. Specifically, ensure that we do not have a registered designer attribute (i.e. comment it out), then enable the power-saving-feature (then maybe restart vs?) then add the attribute back in. We shoudl see the file does not look designable. If we can still open the designer though in a fairly simple/intuitive fashion, i would be ok with that.

I'd just like to see what that flow visually looks like though. Thanks!

else
{
Enable();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could ths be racey, if many options change at once.

// Also re-enable the remote host service which was disabled earlier.
ServiceFeatureOnOffOptions.LowMemoryForcedPowerSaveMode = false;
_remoteHostService?.Enable();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these racey at all?

@CyrusNajmabadi
Copy link
Member

Again, note that we are just brainstorming on initial set of defaults to fit into this power save philosophy

I think that's fair. However, just my 2c, but the model here doesn't fit how i think other systems do "battery mode", so i think it may be confusing and counterintuitive to users.

@CyrusNajmabadi
Copy link
Member

Note: i strongly agree with "I believe the idea here was that most of the features become pay for play". However:

  1. We just need to validate that the above is actually true. i.e. we're not actually killing a feature and making it non-functional when teh user actually wants to use it. :)

  2. that the cure isn't worse than the disease. For example, say i decide that i want nav-to to be pay-for-play (which i can totally get behind). If i then run nav-to and it totally bogs down my VS horribly because it's churning the GC in proc, that's not good. It would still be great to remote it to the OOP server (even if it has to do work) so that it can run without impacting vs.

does that make sense?

@mavasani
Copy link
Contributor Author

We just need to validate that the above is actually true. i.e. we're not actually killing a feature and making it non-functional when teh user actually wants to use it. :)

That is completely reasonable. My first take was to take perf traces and look at everything that was running on the OOP process and start turning them off. Obviously this doesn't tell me if it ended up hurting performance in lot of common user scenarios. So we plan to test this much more rigorously + rely on those who know about performance of these IDE features (you, Heejae and Sam) and keep iterating to make it more lenient or strict on what needs to execute by default to avoid degrading the experience. This PR will likely be around for some more time before we have a reasonable and well tested state for this mode.

@mavasani
Copy link
Contributor Author

Were all the loc changes intentional?

There were merge conflicts in the xlf files and I just rebuilt all the xlf files, hoping it would do the right thing. Well doesn't seem that happened... I'll try to fix it up.

@@ -1358,4 +1358,10 @@ I agree to all of the foregoing:</value>
<data name="Naming_rules" xml:space="preserve">
<value>Naming rules</value>
</data>
<data name="Enable_power_save_mode" xml:space="preserve">
<value>Enable power save mode (disables all background analysis)</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this behave more like the Windows power saving options? i.e. Enabled power saving mode only when we are on battery power?

@CyrusNajmabadi
Copy link
Member

This PR will likely be around for some more time before we have a reasonable and well tested state for this mode.

I completely understand. All my comments are in support of creating a kick ass solution here :) I'll try to provide any insight i can, as well as my thoughts on the best way to design/impl things.

I think this is a great idea and i'm super excited to see where it goes. My persona is simply that i just into feedback right from the start to get all the ideas and mental juices flowing :D

LMK how else i can help. Thanks!

@CyrusNajmabadi
Copy link
Member

There were merge conflicts in the xlf files and I just rebuilt all the xlf files, hoping it would do the right thing. Well doesn't seem that happened... I'll try to fix it up.

No worries. I just had an initial heart attack at the PR side until i realized it was a loc snafu :D

@@ -1358,4 +1358,10 @@ I agree to all of the foregoing:</value>
<data name="Naming_rules" xml:space="preserve">
<value>Naming rules</value>
</data>
<data name="Enable_power_save_mode" xml:space="preserve">
<value>Enable power save mode (disables all background analysis)</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(disables all background analysis)

💡 This is incorrect today, and may change in the future. To avoid confusion, it's probably best to leave it out.

Suggested change
<value>Enable power save mode (disables all background analysis)</value>
<value>Enable power save mode</value>

storageLocations: new RoamingProfileStorageLocation($"Options.DisableAnalyzers"));

/// <summary>
/// Option to turn off all background analysis to improve performance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❕ Power save mode is not designed to improve performance, and is not limited to background analysis. This comment should be rewritten to correctly state the intent of the feature, e.g.:

Option to disable implicit background operations to reduce CPU power consumption.

storageLocations: new RoamingProfileStorageLocation($"Options.PowerSaveMode"));

/// <summary>
/// Enables forced power save mode when low VM is detected to improve performance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❕ Power save mode is not designed to improve performance and not designed to reduce memory consumption. Low VM is not a reasonable automatic trigger for this feature. If we want an automatic trigger, it should be tied to the operating system's current power mode.

Note that I already have a hard-coded exception in the low VM monitor to exclude it from running on my machine, but we really should not be making this change for other people either.

Copy link
Contributor Author

@mavasani mavasani Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to start a separate offline email thread on this - I think we need to discuss two modes here:

  1. Power save mode: Only about battery/CPU consumption, without any aim at reducing memory consumption or turning off potentially expensive features. This can be tied to OS power mode if required.
  2. Lightweight edit mode or resource saver mode: Turn off any background analysis that can lead to high CPU consumption or help reduce memory consumption. It is also reasonable to turn off features in this mode if they are not an absolute requirement for minimal IDE editing experience. Given the memory save characteristics of this mode, it is also likely that some explicitly triggered IDE features might perform worse performance-wise, we would look at these case by case basis and tune the things that get turned off. This mode can be tied to OS power mode and/or low VM trigger.

My PR is basically targeting the latter, and based on my discussions with @heejaechang @jinujoseph and @vatsalyaagrawal later is what the customers have really been requesting. Each of the above modes serve different purposes and we should meet up to decide the priority for these.

@CyrusNajmabadi
Copy link
Member

I am going to start a separate offline email thread on this

:(

@CyrusNajmabadi
Copy link
Member

and based on my discussions with @heejaechang @jinujoseph and @vatsalyaagrawal later is what the customers have really been requesting.

Note: this is very hard to know at times. For example, users may often blame something for something they perceive. For example, it's totally possible for VS to be slow for random reasons unrelated to Roslyn's work. However, at the same time, we are routinely bombarding them with notifictions in the task-list saying "roslyn is doing all this work". Users will often just jump to a conclusion that this work is at fault and that they want it off.

i agree that users want VS to behave better. But we need to responsibly determine which features are truly at fault and drive things appropriately from there. Not necessarily provide features which may be a placebo but may not have any effect at all.

For example, imagine someone turns this on, and we now disable all this stuff, but things are still slow because it's some other feature tying up the UI thread inappropriately. The user didn't really gain anything here.

--

Note: i'm in support of further discussions here. And i think there's a place for a lightweight mode. Let's just make sure we're doing it for the right reasons and not as a reaction to users being unhappy about something else entirely.

@heejaechang
Copy link
Contributor

heejaechang commented Sep 25, 2019

why don't we see what user want? have both options - "Power save mode" or "Lightweight VS mode" and see what option users like more?

make the option shows up for all internal users + some external users who wants it and send notification (yellow bar) asking trying this mode? (we have been using this for fxcop and other features, so we know how to do this. so won't be much extra work to do)

it might save a lot of time arguing over ones preferences.

it might turns out that users want both? or doesn't matter as long as we provide something and we are arguing over something nobody but some people care.

@heejaechang
Copy link
Contributor

@jinujoseph shouldn't we also ping some platform people to see their opinion?

@jinujoseph
Copy link
Contributor

@heejaechang i think we should start small just with in C# options to begin with. I have informed Hem regarding this feature, we can discuss about it when we meet today

@mavasani mavasani modified the milestones: 16.4, 16.5 Oct 10, 2019
@mavasani
Copy link
Contributor Author

mavasani commented Nov 21, 2019

Closing in favor of #38429 #39699

@mavasani mavasani closed this Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict background analysis scope to reduce CPU consumption in live analysis
6 participants