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

[WIP] Project Load improvements #2802

Closed
wants to merge 13 commits into from

Conversation

cloudRoutine
Copy link
Contributor

It'd be great if I could get some additional eyes to review this as I work towards addressing #2793

This first change is to simplify an initial project loading process that seems unnecessarily complicated. I've opened a bunch of slns after making the change, they all seem to open around the same speed or slightly faster, I haven't noticed anything breaking yet, but maybe the CI will catch something...

@majocha
Copy link
Contributor

majocha commented Apr 5, 2017

Apparently there were some slowdowns when doing this. That's why the code is so convoluted. It waits for full solution load.

However I'm all for simplifying this.

@cloudRoutine
Copy link
Contributor Author

@majocha I've found that I just need to build a new way to add the project references into the fsharpprojectoptions being created and then the references to LanguageService & LanguageService.Base can be removed from FSharp.Editor.

initialize has been cut down to

    override this.Initialize() =
        base.Initialize()

        this.Workspace.Options <- this.Workspace.Options.WithChangedOption(Completion.CompletionOptions.BlockForCompletionItems, FSharpConstants.FSharpLanguageName, false)
        this.Workspace.Options <- this.Workspace.Options.WithChangedOption(Shared.Options.ServiceFeatureOnOffOptions.ClosedFileDiagnostic, FSharpConstants.FSharpLanguageName, Nullable false)
        
        this.Workspace.DocumentClosed.Add <| fun args ->
            tryRemoveSingleFileProject args.Document.Project.Id 
            
        Events.SolutionEvents.OnAfterCloseSolution.Add <| fun _ ->
            singleFileProjects.Keys |> Seq.iter tryRemoveSingleFileProject
        
        this.Workspace.WorkspaceChanged |> Observable.add (fun args ->
            match args.Kind with
            | WorkspaceChangeKind.ProjectRemoved ->
                projectInfoManager.ClearProjectInfo args.ProjectId
            | _ -> ()
        )
        
        let theme = package.ComponentModel.DefaultExportProvider.GetExport<ISetThemeColors>().Value
        theme.SetColors()

SyncProject is out, SetupProjectFile is out SetupNewTextView doesn't use the project site anymore.

It's not doing great on @saul's Dense generated project when it's been cut down to 50projs, so I've still got a ways to go with this

@cloudRoutine
Copy link
Contributor Author

Turns out it was a biiiiit more complicated than I initially thought 😆
I think the introduction of the ProjectTracker in a few more places may help with the synchronization, but I need to do some more experimenting to find out for sure

Depending on how well the solution event advising and project tracker keep things running in the background and hooked up to the workspace, it might be possible to cut out a few more large chunks of the current logic.

All usage of ProjectSitesAndFiles has been removed,

  • abstractProjects need to be disconnected and disposed properly
  • FSharpProjectOptions need the rest of their data filled properly (compiler flags etc)
  • Try to make more of the initialization and loading aync or deferred
  • Testing for ProjectOptionsCache
  • Fix SingleProjectFile, Script FSharpProjectOptions generation and ComputeSingleFileOptions

Copy link
Contributor

@saul saul left a comment

Choose a reason for hiding this comment

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

Can we minimise the diff? So much code has just been moved around and it makes it hard to see what's actually changed

self.SetupProjectHierarchy args.Hierarchy |> ignore
)
for project in solutionDTE.GetProjects () do
self.SetupProject project |> getProject |> projectInfoManager.AddProject
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to wait on solution close, and put this in a while loop (just as the old code did).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does it need to wait on solution close? i didn't really understand that part

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly performance was worse with projects being setup during solution load. We don't want this the first time and also we want to avoid it on subsequent solution openings.

Copy link
Contributor

Choose a reason for hiding this comment

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

I admit it was done in a crazy way. Probably using UIContexts would be better.



member self.ProjectTracker : VisualStudioProjectTracker =
match projectTracker with
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use lazy instead?

tryRemoveSingleFileProject args.Document.Project.Id

Events.SolutionEvents.OnAfterCloseSolution.Add <| fun _ ->
singleFileProjects.Keys |> Seq.iter tryRemoveSingleFileProject
Copy link
Contributor

Choose a reason for hiding this comment

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

That was here, because workspace doesn't trigger DocumentClosed on open files when the solution closes. There was some bug associated with it IIRC.

@cloudRoutine
Copy link
Contributor Author

@davkean I've been told you know how to disable/mute the msbuild output in the debug pane. If I could get rid of that noise it'd make working on this less painful.

@davkean
Copy link
Member

davkean commented Apr 13, 2017

@cloudRoutine TBH, I have no idea how F# project system calls MSBuild or who puts the logs into the Output window. The legacy C# project system calls into the default IVsBuildManagerAccessor and I didn't think it logged to the debug pane.

@davkean
Copy link
Member

davkean commented Apr 13, 2017

Can you reverse the layout/formatting changes (submit them in a different review to make them) to make the diff easier to read?

@cloudRoutine
Copy link
Contributor Author

@davkean don't worry about it, I've been getting nowhere with this

@saul
Copy link
Contributor

saul commented Apr 13, 2017

@cloudRoutine a bit late (considering you've closed this), but I know where you can look for this. See ProjectNode.SetDebugLogger in ProjectNode.cs in the base project project.

@cloudRoutine
Copy link
Contributor Author

@saul thanks for the tip, but I have no intention of coming back to this

@saul
Copy link
Contributor

saul commented Apr 13, 2017

Any particular reason? I plan on tackling the project load performance issues soon as it's crippling us at work. Any tips would be great :)

@cloudRoutine
Copy link
Contributor Author

cloudRoutine commented Apr 13, 2017

Cause it's miserable to work on, the integration with lower layers of the project system were deeper than I originally realized and there's a bunch of underlying machinations in the languageservice and languageservice.base that I didn't get into thoroughly investigating.

The interactions between the solution and the workspace are obtuse, and the projectsitesandfile implementation is an odd one I was hoping to be able to factor out, but it seems like it'd take a lot more digging and untangling than I'd already done.

I'd thought I'd made some progress here, but I couldn't figure out why the project references weren't loading properly, if you want to try to pick up from here that'd be the first thing to address.

The reactor seems to be starting up when VS is opened, before any solution has been opened, that probably shouldn't be happening.

If you're wondering why I changed the projectinfo manager to work mainly as a wrapper around the projectinfocache, it was to make it easier to add it into a workspace I built to use for roslyn feature tests (I didn't get around to PR'n it though)

@saul I'll try to answer any other questions you have, but I'm not sure I really learned much concretely while working on this 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants