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

OutOfMemoryException when using Package Manager UI #8352

Closed
vsfeedback opened this issue Jul 18, 2019 · 16 comments
Closed

OutOfMemoryException when using Package Manager UI #8352

vsfeedback opened this issue Jul 18, 2019 · 16 comments
Assignees
Labels
Epic Priority:2 Issues for the current backlog. Product:VS.Client Tenet:Performance Performance issues Type:Bug

Comments

@vsfeedback
Copy link

vsfeedback commented Jul 18, 2019

Using VS nuget GUI to consolodate different versions of a package crashes the instance of VS. Occurs in 2017, 2019 and 2019 int preview.

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/608137/crash-when-consolodating-nuget-packages.html
VSTS ticketId: 918791

These are the original issue comments:

Fiona Niu[MSFT] on 6/17/2019, 01:10 AM (31 days ago):

Thank you for taking the time to log this issue!

I've tried to reproduce and investigate using the description, and attachments already provided. Unfortunately those aren't enough and more information is needed in order to investigate it further.

The easiest way to provide all the information is to use the Visual Studio Feedback Tool. This will ensure that we collect the needed information for you without worrying about what to provide (recording, dump file or ETL trace).

Since this issue is now marked as Need More Info, that workflow is enabled in the Feedback Tool:

  • Open Visual Studio Feedback tool.
  • Click the banner letting you know that you have problems requesting your attention.
  • Click this problem from the list
  • Click "View their request and respond" from the problem details banner
  • Add a comment, in the Attachments/Record: click Start Recording
  • When the Steps Recorder tool appears, perform the steps that reproduce the problem.
  • When you're done, choose the Stop Record button.
  • Wait a few minutes for Visual Studio to collect and package the information that you recorded.
  • Submit. You will be able to see the comment on Developer Community. For security reasons, your files come directly to us and don't appear on Developer Community.

For the full instructions, please see: https://docs.microsoft.com/en-us/visualstudio/ide/how-to-report-a-problem-with-visual-studio-2017?view=vs-2017#when-further-information-is-needed-need-more-info . For information about what data is collected, see https://docs.microsoft.com/en-us/visualstudio/ide/developer-community-privacy?view=vs-2017#data-we-collect

We look forward to hearing from you!

brynhr [MSFT] on 6/17/2019, 02:25 AM (31 days ago):

Recording of crash attached. Please let me know if it uploaded.

Fiona Niu[MSFT] on 6/17/2019, 06:48 PM (30 days ago):

Thanks a lot for providing the information. We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

These are the original issue solutions:
(no solutions)

Internal issue with data

@zivkan zivkan changed the title Crash when consolodating nuget packages OutOfMemoryException when using Package Manger UI Jul 18, 2019
@zivkan
Copy link
Member

zivkan commented Jul 19, 2019

From the solution that reproduces the issue, there's 138 projects (although about 4 wouldn't load on my computer), 20 package sources and 290 packages used in the solution. The first time I loaded the solution, Visual Studio was using about 700MB of memory and when I opened the Package Manager UI and changed the package source to "All", memory ballooned until the process crashed.

When analyzing the original crash dump, there were nearly a million NuGetVersion objects in memory (possibly more, the memory dump was partially corrupt due to a GC running at the time it was captured), which is unreasonable, even given the number of packages, projects and sources.

I don't know how relevant it is, but other times when I open the solution and otherwise do absolutely nothing (no windows open within VS), VS would be very sluggish and frequently would have the "(not responding)" message. I assume it's related to automatic restore and design time build.

@zivkan zivkan added the Tenet:Performance Performance issues label Aug 28, 2019
@nkolev92 nkolev92 added the Priority:2 Issues for the current backlog. label May 4, 2020
@erdembayar
Copy link
Contributor

@zivkan can you share me your solution with 138 projects if possible? I don't know how to recreate this issue on my local. I used VS2017 with many different projects for 2 years never had that issue, so hard to reason what is wrong there.

@erdembayar erdembayar changed the title OutOfMemoryException when using Package Manger UI OutOfMemoryException when using Package Manager UI May 22, 2020
@nkolev92
Copy link
Member

Think we'll do this in S172 (or maybe reevaluate in S171 if we decide so.)

@erdembayar
Copy link
Contributor

erdembayar commented Jun 18, 2020

So far below things I found out for investigation of this bug.
For this I had discussion with my Nuget client team members and Joel Verhagen from Nuget server team.
Also I analyzed memory usage on my machine with solution with 138 projects.

  1. Azure Dev ops to enable Etag (Nuget.org already has it) for server side caching. Also make sure gzip is enabled not clear if it's there.
    Most likely can be done near term, it's just server configuration.
    Otherwise we're downloading same 1GB data over and over for every 30 minutes once local cache expires. It's very inefficient thing and Nuget.org already support Etag. - Add E-tags to V3 protocol  NuGetGallery#8048
  2. Nuget.org should support Etag without "x-ms-version" hack.
    Most likely can be done near term.
  3. Azure Dev ops Registration index page: Make it paged for best practice. Once it reach 128 it creates new page.
    But doesn't decrease amount of data we download, maybe don't see any possitive effect this change.
  4. Protocol change (then Azure Dev op team need to adopt it): Prune information from Registration page (for example @context).
    We can have more discussion on this approach. Only issue with this approach is it will take a long time implememt protocol change and Azure Dev op adopt it.
  5. Replace JsonParse, remove JObject part with strong type.
    Currently this part inside NugetClient, so I can immediately start working on it and I'll looking into ways to bypass JsonParse/JObject.
  6. Ask from Internal repo Nuget or other publishers to remove/prune old ununused versions from Registration index page, currently Internal repo Nuget feed have more than 4k version for each package.
    Most likely they can do in near term. Another problem is find out who are they so we can notify them?

@erdembayar
Copy link
Contributor

Currently I am working on point 5.

@nkolev92
Copy link
Member

nkolev92 commented Jun 19, 2020

Thanks for the update @erdembayar

A few notes on your 5 points.

Point 1: This will affect the network throughput rather than OOM. We have a tracking issue on NuGetGallery side. Keep in mind, protocol changes take more scrutiny.
Please create a tracking issue to consider consuming etags on NuGet side.

Point 2: Can you clarify this? x-ms-version, can you make sure there's a tracking issue on gallery side.

Point 3: I think it's a recommendation. We can communicate it to them, but won't change much in the grand scheme given today's resource usage. See #8058 and #4448.

Point 4:Can you elaborate more on this? The context is meant to be the schema.

Point 5: Sounds great, maybe we need a particular issue for that given that this particular task is investigative and composed of many layers.

Point 6: The registration index is never pruned. Most http servers don't have the concept to completely remove a version. You can only unlist a version. Think we need to do better by doing #8058.

@nkolev92
Copy link
Member

Re #8058
PackageSearchMetadata takes a lot more memory than NuGetVersion.
Registration uses PackageSearchMetadata, while we'd be more efficient if we just loaded NuGet versions

@erdembayar
Copy link
Contributor

Thanks for the update @erdembayar

A few notes on your 5 points.

Point 1: This will affect the network throughput rather than OOM. We have a tracking issue on NuGetGallery side. Keep in mind, protocol changes take more scrutiny.
Please create a tracking issue to consider consuming etags on NuGet side.

Point 2: Can you clarify this? x-ms-version, can you make sure there's a tracking issue on gallery side.

Point 3: I think it's a recommendation. We can communicate it to them, but won't change much in the grand scheme given today's resource usage. See #8058 and #4448.

Point 4:Can you elaborate more on this? The context is meant to be the schema.

Point 5: Sounds great, maybe we need a particular issue for that given that this particular task is investigative and composed of many layers.

Point 6: The registration index is never pruned. Most http servers don't have the concept to completely remove a version. You can only unlist a version. Think we need to do better by doing #8058.

I just to give update on this ticket since I highlighted 6 points.

  1. I created ticket for Point 1 here #9717.
    On recent survey I customers complaining about slow speed, I believe this will help with that
    one, because we're spending time to download json.
  2. Nuget Server team added ticket for etag here.
  3. Discuss later.
  4. Discuss later.
  5. PR for replacing JObject is already on review, according to micro benchmarking it reduces runtime roughly by 40% and memory consumption by 50%.
  6. I only proposed pruning unused very old versions for internal customers if they reach out to us about PM UI problem. There are no need to keep 4k different version for each package, it gets worse if there are 230 such packages on same solution such as this one, it's just dead weight.

@nkolev92
Copy link
Member

nkolev92 commented Jun 26, 2020

  1. Sounds like you sync-ed up with @joelverhagen, issue here: NuGet.org V3 API endpoints should return a HTTP compliant etag NuGetGallery#8071

  2. I've created another issue: Improve memory performance of PackageMetadataResourceV3 by reducing the JObject dependencies #9719 for that. We can leave this issue tracking the general work.

  3. Deleting packages from a feed is not recommended, in fact nuget.org does not allow it whatsoever. https://docs.microsoft.com/en-us/nuget/nuget-org/policies/deleting-packages. Some private feed providers will allow that, but that's very often not a realistic option.

I've converted this issue to an epic.

@nkolev92 nkolev92 added the Epic label Jun 26, 2020
@abelbraaksma
Copy link

abelbraaksma commented Jun 27, 2020

In case it helps: for a (simpler) repro of this issue with only 20 projects, open the FSharp.sln from the F# repo. To get VS 2019 to disappear and/or show the crash window:

Now go to FSharp.Core.UnitTests and open the "Manage NuGet packages", click "Browse". Then wait for it... wait for it... BOOM!™

I've had a few times that it shows a generic error in that window instead of a crash, that nonetheless everything got extremely slow, and then during analyzing the issue, I saw that all memory was used (3GB+).

Why this window needs 2.5GB memory or more is beyond me, and I don't really understand where the cause lies, in the end, only a fraction of that should suffice, I guess, for polling partial display lists etc etc.

Originally reported here: dotnet/fsharp#9518

PS: I'm very happy to see that work is underway on this complex issue!

@zivkan
Copy link
Member

zivkan commented Jun 27, 2020

@abelbraaksma Looking at fsharp's nuget.config, it uses .NET Core's sleet feed. Since sleet is a static feed generator, and blob storage is a static file server, and given how many packages are in the feed, this means the search results url is a 230 megabyte json file, despite the fact that the HTTP get parameters ask for 25 results (afterall, it's a static file).

When you open the package manager UI, if you do not use "dotnet-core" or "all" in the source dropdown, you shouldn't have out of memory crashes.

This PR fixed that issue (and should be available in the next Visual Studio 2017 16.7 preview, if it's not in the current preview), by ignoring any search results over the number expected in the result. This will mean, however, that only the first 25 package ids will be browseable, and will be the search results regardless of search query. The NuGet protocol wasn't designed for purely static feeds, the search service at a minimum needs to be dynamic. But I'm reasonably confident it will fix the crash you're experiencing.

@abelbraaksma
Copy link

abelbraaksma commented Jun 28, 2020

Visual Studio 2017 16.7 preview, if it's not in the current preview

2019, I hope? 😄

it uses .NET Core's sleet feed. Since sleet is a static feed generator, and blob storage is a static file server, and given how many packages are in the feed, this means the search results url is a 230 megabyte json file, despite the fact that the HTTP get parameters ask for 25 results (afterall, it's a static file).

Ouch! I had no idea! So they should fix this on MS's side to make it non-static? Though I guess that's not gonna happen anytime soon.

@zivkan, many thanks for your quick and insightful comment. I'll be sure to test the next Preview release soon. Loading 230MB on each GET request sounds massive, no wonder it took so long (and then crashed)!

@erdembayar
Copy link
Contributor

erdembayar commented Jun 29, 2020

With my latest PR finally I was able to open PM “Consolodate UI” and “Update UI” without crash (see attached screenshot), We’re expecting this fix will go out with 16.7 release. It’s end of all, we need to make more performance improvement.

image

@nkolev92
Copy link
Member

Reducing priority as we've addressed the highest impacting issues.

@nkolev92
Copy link
Member

Closing this for now.

We've done a bunch of work, and while we're never perfect, I don't see value in tracking this issue anymore, given that the traces in 16.7 are going to be vastly different.

For anyone reading this issue and running into this problem, please try it with VS 2019, Update 7 (shipping soon), and if you are still having issues, we'd be happy to hear about your scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic Priority:2 Issues for the current backlog. Product:VS.Client Tenet:Performance Performance issues Type:Bug
Projects
None yet
Development

No branches or pull requests

6 participants