search

scientistproject / Scientist.net

A .NET library for carefully refactoring critical paths. It's a port of GitHub's Ruby Scientist library
MIT License
1.46k stars 95 forks source link

Added AllocatedBytes metrics to Observations #111

Open Lexcess opened 6 years ago

Lexcess commented 6 years ago

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 commented 6 years ago

Can anyone take a look?

haacked commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

joncloud commented 6 years ago

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 commented 6 years ago

@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)

haacked commented 6 years ago

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.

Lexcess commented 6 years ago

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 commented 5 years ago

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: https://github.com/github/Scientist.net/issues/114