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

MemoryDiagnoser should include memory allocated by all Threads that were live during benchmark execution #723

Closed
adamsitnik opened this issue Apr 18, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@adamsitnik
Copy link
Member

commented Apr 18, 2018

Suggested by @stephentoub

Another issue I have with the memory columns is the allocation one. For async methods, it's usually very misleading, since it's only measuring the calling thread.
So I'm manually removing that from the results where it doesn't make sense.
I'd suggest the memory diagnoser by default remove it as well when the benchmark returns Task
But have some way to turn it back on
Or something like that

Current problem: there is no API available for .NET Core that we could use. As of today BDN is using System.GC.GetAllocatedBytesForCurrentThread() for .NET Core. There is no way to iterate through all the threads and get this value. Moreover, some threads could have exited in the meantime.

The problem does not occur for .NET framework, where we use AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize

@haf

This comment has been minimized.

Copy link

commented May 14, 2018

This is an issue for me when trying to benchmark Logary, because all GC happens on Hopac threads. However, these threads are known at runtime during setup: https://github.com/Hopac/Hopac/blob/master/Libs/Hopac.Platform/Init.fs#L13

Is there are way to tell BDN about known threads?

@adamsitnik

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

Is there are way to tell BDN about known threads?

No, but if you run your benchmarks for .NET (not Core) for which we are using different API (which does not have this issue) the allocated memory includes memory allocated by all threads.

Unfortunately, there is nothing we can do on our side, we need a new method to be exposed by CoreCLR.

@NickCraver

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

I've fired up the API discussion in dotnet/corefx#30644

@hpbieker

This comment has been minimized.

Copy link

commented Mar 12, 2019

It would be nice to get this fixed so that we could get proper memory numbers for async code where some of the callbacks ends up on some other thread.

@adamsitnik

This comment has been minimized.

Copy link
Member Author

commented May 17, 2019

This issue has been fixed in #1155 but it's going to work only with .NET Core 3.0 preview6+.

The .NET Team does not plan to backport the new API to older versions of .NET Core.

@gfoidl

This comment has been minimized.

Copy link

commented Jun 4, 2019

Should https://github.com/dotnet/performance/blob/master/docs/benchmarkdotnet.md (L233)

  • For .NET Core the Allocated Memory is only for the current thread, see #723 for more.

be updated to reflect this?

@adamsitnik

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

@gfoidl yes, good catch! Could you please send a PR?

@gfoidl

This comment has been minimized.

Copy link

commented Jun 4, 2019

Yes, will send one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.