Added AllocatedBytes metrics to Observations

[Lexcess]

Hi,

I like the idea of Scientist.Net, as I often don't want to set up a full Benchmark.Net run. However I really felt the loss of the memory allocation information that I could get from Benchmark.Net.

This Pull Request has a working proposal (with unit tests) for a new AllocatedBytes field on Observation. To enable this to work cross runtime I've cribbed the workarounds from Benchmark.Net's implementation (Garbage Collection tooling is wildly inconsistent - see the last paragraph). Included are a test for the basic functionality and one that asserts that Scientist's own allocations don't appear in the results. Note that I did not include Generational counts because it seemed trivially easy to get miscounts given that Scientist.Net works 'inline' versus Benchmark.Net's dedicated test runs.

I also upped the versions of the Nuget package (for my testing), the Core SDK (for the exe/unit tests) so they used the latest GC Tooling. You can revert these changes and the code will still work (the unit tests will need slight modification though).

In terms of impact of adding this behaviour. For .Net Core it is almost imperceptible, for .Net Framework the only way to achieve this functionality is with GC.Collects which is a little more invasive. I hope that the benefit of seeing allocations outweighs any perceived downsides.

Lastly I've proposed an addition to vNext of .Net Standard that would do away with all the hackery for both this, Benchmark.Net and anyone else who cares about allocations at runtime.

Commit notes

Added target for .Net Standard 2.0 to detect better GC tooling
Added test for new AllocatedBytes feature
Added test to account for Scientist's own allocations in AllocatedBytes
Upgraded test and console app to netcoreapp 2.1 to use new GC Tooling
Some portions of GCHelper were derived from Benchmark.Net and are used under license

[Lexcess]

Can anyone take a look?

[haacked]

Thanks @Lexcess! I've been on vacation the last two weeks, hence the delay. Are you currently using this in production anywhere? @joncloud, @martincostello, @ryangribble do any of you have time to look at this too?

I'll try and find time to review it this week, but would welcome other eyes on it.

[haacked]

For .Net Core it is almost imperceptible, for .Net Framework the only way to achieve this functionality is with GC.Collects which is a little more invasive.

This is a concern. Perhaps we should add a flag to enable this. That way it's off by default and for those who want to track allocated bytes, they can turn it on easily.

[joncloud]

Sure thing - I'll try and provide some feedback where I can.

[joncloud]

What do you think about taking this check outside of this method, and use it during the creation of GetAllocatedBytesForCurrentThreadDelegate? We'd be able to save the check, and then always just run

GC.Collect();
return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;

[Lexcess]

So do it purely on #IfDef? The reason we do a runtime framework check is because the if you used the code within a dotnet Standard library you wouldn't know at compile time your runtime (FX, Core or Mono). Apologies if I've missed your point here I'm not convinced that GitHub is pointing me to the right line.

[joncloud]

Yeah - this may have been a little misleading. What i'm suggesting is moving this logic out to where we construct the GetAllocatedBytesForCurrentThreadDelegate delegate. I'll add some comments in that area to help facilitate.

[Lexcess]

See below, I think this is addressed with your later comment.

[joncloud]

This seems fairly solid, however I am definitely not a master of memory. I think @haacked has a good idea by opting into this behavior. Looking at how you have this implemented I think that could be pretty easy to transition to configuration.

[Lexcess]

@haacked @joncloud I would be fine with it being opt in, I think you'd need to give some steer on how you'd want that to work.

Just as a counterpoint though, I'm hopeful that .Net FX will incorporate per thread allocation data at some point (the Mono team just made a soft commitment to do so on their thread tracking this). So if the issue is the GC.Collect behaviour then perhaps it would be better to add the flag around that rather than the feature itself? In terms of benchmarking memory allocation data is far more reliable than timings so it might be a shame to have it only opt in when the issue is more around one platform (and a platform where perf is less of a focus anyway).

Again though if you think opt in is the model if you could describe how you'd like it to be enabled then I'll make the change. Could we leave the field on Observation in either case? It would make doing publishing a lot easier (otherwise I guess we'd need it to be subclassed or perhaps a dictionary for optional data)

[martincostello]

Just checking, these aren't strings that can vary if language packs are installed on the local machine?

[Lexcess]

I don't believe the Framework descriptors are localized, and don't have an easy way to check. I lifted this behavior from Benchmark.Net so I suspect they would have already snagged on this if it was a problem.

[Lexcess]

Also Hey Martin, I thought that looked like your name in the commits but thought it was a coincidence.

[martincostello]

👋

[haacked]

So if the issue is the GC.Collect behaviour then perhaps it would be better to add the flag around that rather than the feature itself?

I could live with that.

[joncloud]

This is a continuation of the comment above. I think we can implement some runtime logic here:

if (IsFullFramework) {
  return () => {
    GC.Collect();
    return AppDomain.CurrentDomain.MonitoringTotalAllocatedMemorySize;
  };
}
else {
  return () => 0L;
}

This will move the IsFullFramework check out from the method call itself, and only check during the first initialization run.

[Lexcess]

Ah I see, cache off the delegate like we do with .Net Core. I think at one point that wasn't possible (due to some additional GC.Collects), but you're right we can make this change. I'll try and find time to make the changes in the next day or so.

[Lexcess]

So I've gone through and reworked the code. It does mean that different implementations are used to get byte counts (process wide versus current thread). However the perf tradeoff might be worth it. I've also bundled in the allocation quantum logic in the delegate.

[Lexcess]

I've applied the PR feedback and got the tests passing. I haven't added a flag to enable opt in for NetFX's GC.Collect impl. Did you have a preference in mechanism for that? I wouldn't recommend config file dependencies on a library. Other than that there are numerous options we could #IfDef in a NetFX only overload or have a platform agnostic flag that only comes into play with NetFX. Any thoughts?

[shiftkey]

Just wanted to bump this PR because it seems to have stalled.

I'm looking for new maintainers to take over this project, and if you'd like to get involved please chime in over there: #114