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

Debugger.NotifyOfCrossThreadDependency can be slow when a debugger is attached #38736

Closed
gregg-miskelly opened this issue Jul 3, 2020 · 46 comments · Fixed by #101864
Closed
Assignees
Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions in-pr There is an active PR which will close this issue when it is merged tenet-performance Performance related issue
Milestone

Comments

@gregg-miskelly
Copy link
Contributor

Description

Certain code paths will call Debugger.NotifyOfCrossThreadDependency. NotifyOfCrossThreadDependency is pretty close to free when there is no debugger attached, so if you are in a situation where you do something cheap which calls NotifyOfCrossThreadDependency then you can have a very high overhead for the with debugger case compared to the without case. Recently one such case surfaced with Task completion: https://developercommunity.visualstudio.com/content/problem/1097088/attaching-debugger-to-heavily-async-process-causes.html

Ideally, we would avoid notifying the debugger if NotifyOfCrossThreadDependency was called when a func-eval wasn't happening.

Stack

@gregg-miskelly gregg-miskelly added the tenet-performance Performance related issue label Jul 3, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Diagnostics-coreclr untriaged New issue has not been triaged by the area owner labels Jul 3, 2020
@ghost
Copy link

ghost commented Jul 3, 2020

Tagging subscribers to this area: @tommcdon
Notify danmosemsft if you want to be subscribed.

@Mithras
Copy link

Mithras commented May 4, 2021

Here is a working workaround that might be ok for some scenarios (e.g. dev containers): #52149 (comment)

@hoyosjs hoyosjs modified the milestones: 6.0.0, 7.0.0 Jul 26, 2021
@NickCraver
Copy link
Member

@hoyosjs Is there a chance this can not be pushed out to 7.0 but fixed in .NET 5 instead? We're seeing this at Stack Overflow frequently now, causing devs a lot of pain with locked-up debuggers (only killing the process works). IMO this is impacting work enough to warrant a patch fix for those it's affecting.

It's not happening for everyone, but we can reproduce this often. Is there something we can assist with in getting to the bottom of the issue? In the repros my team managed here (we started looking because of other dev reports), the symptoms in this issue with Debugger.NotifyOfCrossThreadDependency is our most likely culprit given the stacks at the time.

cc @deanward81

@hoyosjs
Copy link
Member

hoyosjs commented Aug 12, 2021

@NickCraver Thanks for the report. I will plan to do this early in 7.0 and try to justify a backport (given that this is a debug time perf thing it'll be hard to justify, but if it's causing unreasonable pain I might be able to make a case). Maybe I can give you a private build when I do and you can let me know if it solves the issues y'all are seeing?

@colonelchlorine
Copy link

I'm also experiencing this issue when debugging large application in VSCode. If there's a lot of async/await activity going on in our project, and it can lock up performance in NotifyOfCrossThreadDependencySlow (found in dotnet-trace). Replacing System.Private.CoreLib.dll with a fresh build that has Debugger.NotifyOfCrossThreadDependencySlow() commented out, perf goes back to normal.

@dkattan
Copy link

dkattan commented Nov 2, 2021

This is causing huge issues for me and my team as well.

@hoyosjs
Copy link
Member

hoyosjs commented Nov 2, 2021

Thanks for all the feedback. I've slated this as needed work for .NET 7. Once we have this in as it is properly tested, I'll take it through the process to request it to be backported to 6.0. I can hopefully have it done in a low risk manner to get it through.

@amandal1810
Copy link

I am seeing this issue on .NET 6.0.100 on Visual Studio 2022 Current on a Service Fabric project. The problem I face is exactly same as described in #42375. Is there any workaround for this? Its heavily impacting productivity.

@tommcdon
Copy link
Member

@amandal1810 To help us rule out other factors that may be impacting debugging, would you mind trying to set the COMPLUS_TieredCompilation environment variable to 0?

@Mithras
Copy link

Mithras commented Nov 15, 2021

Here is a working workaround that might be ok for some scenarios (e.g. dev containers): #52149 (comment)

this is the only workaround

@amandal1810
Copy link

Thanks @tommcdon but I've already tried it as per the comment I saw here. But it did not help.

@amandal1810
Copy link

Thanks @Mithras. I saw that comment as well, but I am not sure how to achieve it for my Service Fabric application. Would appreciate any help.

@hoyosjs
Copy link
Member

hoyosjs commented Nov 15, 2021

@amandal1810 what's your use of service fabric? Is it SDK client classic workload, or is it the azure service. The fix there is not correct - but might help ease the inner loop. What needs to essentially change for that workaround is you need to deploy from a different private build you create. What type of application are you hosting? Self-contained?

@amandal1810
Copy link

@hoyosjs it is an Azure Service Fabric application that contains API services. And yes, it is a self-contained application.

What needs to essentially change for that workaround is you need to deploy from a different private build you create.

Do you mean a custom private build of .NET 6?

@Mithras
Copy link

Mithras commented Jan 7, 2022

In case somebody needs it, here is how I'm building a scratch image with patched binaries:

# https://github.com/dotnet/runtime/blob/release/6.0/docs/workflow/building/coreclr/linux-instructions.md
FROM mcr.microsoft.com/dotnet-buildtools/prereqs:centos-7-20210714125435-9b5bbc2 as builder

WORKDIR /source

ARG COMMIT_HASH
RUN git clone https://github.com/dotnet/runtime.git . \
    && git checkout $COMMIT_HASH

# Comment out NotifyOfCrossThreadDependencySlow();
RUN sed -i 's/NotifyOfCrossThreadDependencySlow();/\/\/NotifyOfCrossThreadDependencySlow();/g' ./src/coreclr/System.Private.CoreLib/src/System/Diagnostics/Debugger.cs

# Build clr (https://github.com/dotnet/runtime/blob/release/6.0/docs/workflow/README.md)
RUN ./build.sh clr -c release

FROM scratch

COPY --from=builder /source/artifacts/bin/coreclr/Linux.x64.Release/sharedFramework /source/artifacts/bin/coreclr/Linux.x64.Release/System.Private.CoreLib.dll /opt/coreclr/

after that you can re-use them in any other image, e.g.

COPY --from=ABOVE_IMAGE_NAME /opt/coreclr /usr/share/dotnet/shared/Microsoft.NETCore.App/6.0.1

@slonopotamus
Copy link

There is almost zero chance it will be fixed in 9.0.0. If you scroll through this issue, it is just constantly delayed. Initial milestone was set to 6.0.0.

@montella1507
Copy link

So... they let all .NET Developers working with multi-threaded environment being doomed? Microsoft Orleans Included?

I did not imagine ANY situation when i was willing to switch back to windows as a developer... and here we are :-)

@dkattan
Copy link

dkattan commented Aug 21, 2023

I'm not sure what we did but this stopped happening for us. Do we have a minimum reproduceable project for this?

@montella1507
Copy link

I am on the latest Orleans na latest public .NET SDK / Runtime...

ON windows i wait 3sec for response, on macOS 70sec... Unfortunately it is not small (minimal repro) project. It is our own calendar server where 1 resource is 1 grain.

@dkattan
Copy link

dkattan commented Aug 21, 2023

I should also note that it wasn't happening to everyone on our team. We all work out of the same codebase. Biggest change was we got everyone new machines which forced a clean install. I know nobody wants that as the solution, but it's worth considering.

Also, moving from 11th gen i9 to 12th i9 was a game changer for build times. 3 minutes to 30 seconds. I wouldn't be the least bit surprised to find that this problem is lower-level than Visual Studio. The general consensus is that it has to do with websockets so I wonder if there's a driver like a VPN or security software in the network stack attempting to intercept these sockets, which could explain why it affected me but not all team members.

@montella1507
Copy link

i am not sure if you read everything in this thread, but this issue is probably connected only with macOS.

@jeff-simeon
Copy link

i am not sure if you read everything in this thread, but this issue is probably connected only with macOS.

It’s not

@montella1507
Copy link

i am not sure if you read everything in this thread, but this issue is probably connected only with macOS.

It’s not

so this is even more imporant than i have ever thought. SO even more sad, how MS is postponing this issue..

@zhenkas
Copy link

zhenkas commented Sep 15, 2023

Here the simple reproduce, running on NET7-WINDOWS in Visual Studio 17.7.4
The following test does benchmark of await Task.Yield() statement

Without debugger on Ryzen 7950x 16 Cores/32 threads it does 3200k op/s
With debugger attached(no breakpoints) it does 23k op/s (150 times slower!!!)

void TestCoroutineTaskRunPerf(int numThreads, int numFlows)
        {
            ManualResetEvent manualResetEvent = new ManualResetEvent(false);
            List<Task> tpTasks = new List<Task>();
            int curThreads = 0;
            var tpAncrease = async () =>
            {
                await Task.Yield();
                if (Interlocked.Increment(ref curThreads) == numThreads)
                {
                    manualResetEvent.Set();
                }
                else
                {
                    manualResetEvent.WaitOne();
                }
            };
            for (int i = 0; i < numThreads; i++)
            {
                tpTasks.Add(tpAncrease());
            }
            Task.WaitAll(tpTasks.ToArray());
            long[] flowCounters = new long[numFlows];
            bool stop = false;
            Func<int, Task> editLocal = async (int index) =>
            {
                await Task.Yield();
                flowCounters[index]++;
            };
            Func<int, Task> loop = async (int index) =>
            {
                await Task.Yield();
                while (!stop)
                {
                    await editLocal(index);
                }
            };
            List<Task> allTasks = new List<Task>();
            for (int i = 0; i < numFlows; i++)
            {
                allTasks.Add(loop(i));
            }
            int testTime = 10000;
            Thread.Sleep(testTime);
            stop = true;
            long totalCalls = 0;
            for (int i = 0; i < numFlows; i++)
            {
                totalCalls += flowCounters[i];
            }
            Task.WaitAll(allTasks.ToArray());
            double speedKPS = (double)(totalCalls) / testTime;
            Logger.LogMessage($"CoroutineTaskRunPerf[{numThreads}/{numFlows}] {speedKPS}k op/s");
        }
        [TestMethod]
        public void CoroutineTaskRunPerf()
        {
//             TestCoroutineTaskRunPerf(1, 1);
//             TestCoroutineTaskRunPerf(2, 2);
//             TestCoroutineTaskRunPerf(4, 100);
//             TestCoroutineTaskRunPerf(8, 200);
 //           TestCoroutineTaskRunPerf(16, 400);
            TestCoroutineTaskRunPerf(32, 800);
        }

Execution pause shows that all threads hang on Critical Section inside NotifyOfCrossThreadDependency

image

Current test does exactly NOTHING except awaiting on Task.Yield(), so the real application that does other things will experience much much more effect from this.

@montella1507
Copy link

montella1507 commented Dec 6, 2023

Hello @slluis! This item is on our backlog and we plan to pick this up and work on it in .NET 8.

SO? Maybe next year september, right

no matter https://github.com/dotnet/orleans is not usable on macos at all...

@tommcdon
Copy link
Member

tommcdon commented Dec 6, 2023

Hello @slluis! This item is on our backlog and we plan to pick this up and work on it in .NET 8.

SO?
Hi @montella1507
We apologize - the fix is more complex than original thought. We are tracking this as part of .NET 9.

mormegil-cz added a commit to mormegil-cz/WikidataBot that referenced this issue Dec 8, 2023
With async, the performance drops terribly when debugger is attached.
(And async is completely unnecessary here, anyway.)

See dotnet/runtime#38736
@ReubenBond
Copy link
Member

ReubenBond commented Dec 20, 2023

Users of Orleans are reporting that this makes debugging their applications unusable on mac OS. Is there anything we can do about this in the library? The original report was also from a team using Orleans internally at Microsoft.

I'd like to understand the problem more. From an initial look (please forgive my naivete), it appears that NotifyOfCrossThreadDependency fires in cases where it might not be needed (where it would have no effect on correctness), impacting performance when a debugger is attached. I realize that it is likely very complicated or impossible to avoid that completely. The cost of handling the notification seems to vary across debuggers and platforms, too - but it's not clear to me how or why.

@tommcdon
Copy link
Member

Hello @ReubenBond! Your analysis is correct. Func-eval's are used for expression evaluation, example calling ".ToString()" to display locals in the debugger. Func-eval's are only executed when the debugger is in break state, meaning that all managed threads are at a GC safepoint and stopped. NotifyOfCrossThreadDependency is used to abort a func-eval that might require multi-threaded execution, for example taking a lock might block indefinitely if owned by a thread that is paused. It signals the debugger to abort the func-eval, and then the debugger (such as VS) will notify the user that the expression evaluation requires letting all threads to slip. In heavily async code, NotifyOfCrossThreadDependency can cause a performance degradation while debugging. The fix will involve only firing NotifyOfCrossThreadDependency when a func-eval is actually in progress.

As for workarounds, customers have reported success building a private coreclr build with NotifyOfCrossThreadDependency disabled, for example #52149 (comment).

@ReubenBond
Copy link
Member

Thank you, @tommcdon. A fix for this would be greatly appreciated. Am I understanding correctly that it's not the notification itself that is expensive, but how the notification is handled, or is it both? I am wondering whether debuggers themselves could choose to ignore the notification when they know it is not relevant. From your explanation, it sounds like debuggers only need to act on the notification when the application is in break mode (or, perhaps also when they are evaluating a trace point, which I suppose is the same for all intents and purposes?).

@tommcdon
Copy link
Member

Hi @ReubenBond. Debuggers should already be ignoring NotifyOfCrossThreadDependency notifications when a func-eval is not in progress. The performance issue is on the coreclr side and so unfortunately I do not believe debuggers can reliably work around the issue without a fix on our side.

@borland
Copy link

borland commented Dec 23, 2023

@ReubenBond indeed, we have found that in our codebase - which is for the most part a stereotypical large web API style application with a lot of MVC controllers and async database access - nothing super fancy -- debugging on macOS is effectively broken without patching the Runtime.

As an example, running a test that takes ~12 seconds without the debugger attached (maybe ~16 with the debugger attached on windows) takes upwards of 3 minutes. Linux experiences the same issue.

So much so that my team maintains a private repo which uses GitHub actions to build patched runtime DLL's so that the rest of the team doesn't have to do this themselves. The patch is literally just commenting out the implementation of NotifyOfCrossThreadDependencySlow; we have experienced no adverse effects from this on either .NET 6 or 8.

@tommcdon Can you please put in a word at Microsoft to fix this for .NET 9, or ideally a patch-release of 8? I understand there will be complex internal discussions involved around correctness, but please consider the cost against the impact, and consider that we, real users are using runtimes which comment out the method entirely. It's not a tradeoff between correctness and performance; the performance is so bad that it's a tradeoff between marginal incorrectness, and a basically nonfunctional debugger. I'll take the marginal incorrectness every time.

@montella1507
Copy link

@borland hi, to others as well. Please, could anyone explain how to use that patched build of .NET ? I was able to build it somehow but did not know how to use it afterward. Was not successfull only with rewriting of files on my mac...

@forenpm
Copy link

forenpm commented Dec 23, 2023

I understand that fixing the root cause is probably nuanced/complex and the reason this keeps getting pushed.

However, something so significantly impacting a large user base should have the workaround cooked in with a simple environment flag (or similar).

DEBUGGER_DISABLE_NOTIFY_CROSS_THREAD = 1

Boom, done. Are there non-obvious challenges to implementing something like that instead of making everyone either give up or struggle with building coreclr custom?

@tommcdon
Copy link
Member

@tommcdon Can you please put in a word at Microsoft to fix this for .NET 9, or ideally a patch-release of 8?

Of course!

workaround cooked in with a simple environment flag (or similar).

We'd be happy to consider this as an option. Has anyone run into issues with disabling NotifyOfCrossThreadDependency? Our concern would be deadlocks which leads to unsafe func-eval aborts (known as "rude aborts").

@Soarc
Copy link

Soarc commented Dec 24, 2023

We'd be happy to consider this as an option. Has anyone run into issues with disabling NotifyOfCrossThreadDependency? Our concern would be deadlocks which leads to unsafe func-eval aborts (known as "rude aborts").

Ownestly funcs evals work only in 1 case of 100 anyway. I personally prefer to have working debugger on mac/linux rather than barly working func evals in watch window

@ForNeVeR
Copy link
Contributor

While I am fully on board with the idea of adding an opt-out of this feature via an environment variable, I would also like to add some ideas to the mix, to consider them after we add a global opt-out switch for that.

As I currently understand, most folks experience issues during general debugging sessions, not while evaluating any methods or variable values or whatnot.

So, now I wonder: if it's possible to enable and disable this feature via an environment variable, could we make the debugger to enter and leave the mode when NotifyOfCrossThreadDependency will work as intended, only during actual variable evaluation?

In this case, any cross-thread dependencies during debugging will still be properly resolved, but the general debugging experience will not be affected.

Could we do that? Does it make sense?

@gaoyang
Copy link

gaoyang commented Feb 1, 2024

This question stalled me for a long time. Is there a convenient remedy.

@montella1507
Copy link

Has anyone found this issue in .NET 9 roadmap?
dotnet/aspnetcore#51834

@mkArtakMSFT @tommcdon

@borland
Copy link

borland commented Feb 29, 2024

@tommcdon

Has anyone run into issues with disabling NotifyOfCrossThreadDependency? Our concern would be deadlocks which leads to unsafe func-eval aborts (known as "rude aborts").

We have, I'd guess, 40-50 developers using .NET on macOS or Linux every day, all of whom have patched the runtime to comment out NotifyOfCrossThreadDependency. My team facilitates this and we've never heard of any problems

@zakeryooo
Copy link

Is this issue still on the roadmap? Just hit it while debugging an async heavy .NET app, took a while to understand what was going on. Patching the framework makes the debugger usable but of course that's not a real solution

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label May 3, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions in-pr There is an active PR which will close this issue when it is merged tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.