-
Notifications
You must be signed in to change notification settings - Fork 36
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
RFC on changes for 0.3 #17
Comments
(no strong opinion on any of the points)
consider publishing
Love this design!
Curious, would it be possible to have some kind of internal lock for this in
|
Hi, thank you for making this library. Since you brought up switching between profiling/non-profiling modes: I have been using this pattern for painlessly switching between profiling & non-profiling: #[cfg_attr(feature = "heapprofile", global_allocator)]
#[cfg(feature = "heapprofile")]
static ALLOCATOR: dhat::DhatAlloc = dhat::DhatAlloc;
pub fn main() {
#[cfg(feature = "heapprofile")]
let _dhat = dhat::Dhat::start_heap_profiling();
... Now I like the new naming and explicit qualification regardless, but what do you think about encoding this pattern either in the library or the docs? |
Good suggestion. Looks like Cargo will just "do the right thing" with that version string? In that case, I can merge my branch to
There is already an internal lock, that's not the issue. Imagine you have two tests.
These two tests run in parallel. Currently, because you can't ever create more than one It might be possible to make unit tests work with the following changes.
Having to rely on
Ok.
Yes, there's a I can see this is confusing to the reader. But it is also very convenient for a couple of the tests. I could just write to file and then have the tests read the file back in, and then delete the file, but that's a bit clumsy. Is there a way to hide that lifetime in rustdoc? Probably not... but I think I can see a way to restructure things so that the lifetime is avoided.
Ok. Instead of seems reasonable, the former is shorter so the latter doesn't seem necessary.
True, I'll change it to Thank you for the excellent comments! Some good improvements will come from them. |
Nice idea! Thank you for the suggestion. I will add something about it to the docs. Just to clarify: you have to add Also I think you can change this part:
to something slightly simpler:
I just tried that and it works. |
I have done this: I have also updated the instructions at the top of this PR accordingly. |
Can we replace the Yeah, I don't see a solution here after all. |
A nuclear option is to store |
Couldn't an alternative be to store stats per-thread? This obviously won't work for tests using global thread pools, but it could be enough to test algorithms running on a single thread or spawning their own dedicated threads. |
Interesting idea, but I can't see how it would work.
So I think the notion of per-thread stats isn't workable. Time-based splitting (i.e. serialization) of profiling tests seems the only way forward. |
Progress update: I will be working through the suggested changes. The ones relating to non-exhaustiveness, naming, lifetimes, and feature flags are simple. The one about enabling units tests will take some more thought. There will be a 0.3.0-pre.2 release at some point incorporating these changes, but it's likely to be in early 2022 after the holiday period. |
palfrey/serial_test#42 is interesting. It suggests using an |
I tried doing unit tests. Unfortunately, making unit test execution serial (e.g. with If you run I guess if you really want unit tests, you can add a |
I have published 0.3.0! Thanks to everyone for the feedback. |
dhat
0.3 will have several major changes compared to 0.2, including API changes. This issue will serve as a place for discussion on these changes, including suggestions for improvements.You can try the
0.3.0-pre.1
release by adding this dependency to yourCargo.toml
:The rust docs can be seen here.
Heap profiling basics
With 0.2, a simple heap profiling example looks like this:
With 0.3, it looks like this:
Explicit qualification is now preferred to
use
statements. This avoids the extrause
line, which helps avoid warnings (or errors) if you are switching between profiling and non-profiling by commenting/uncommenting the#[global_allocator]
declaration and/or thelet _profiler
line. It's also helpful for the newly added assertions, see below.Dhat
is removed from type names to help with this, e.g.DhatAlloc
is renamedAlloc
, because writingdhat::DhatAlloc
is silly.Dhat
is renamedProfiler
. This name better reflects what it is;Dhat
was meaningless. This also works better with the newProfilerBuilder
type, described below. And thenew_heap()
function name is a more typical name for a constructor.Ad hoc profiling basics
With 0.2, a simple ad hoc profiling example looks like this:
With 0.3, it looks like this:
Stats
In 0.2, the
Stats
struct was only intended for internal testing purposes and contained an optionalHeapStats
value, which was an awkward way of handling the heap/ad hoc split. Some of the field names made sense for heap profiling but not for ad hoc profiling. There was a top-levelget_stats()
function to get stats.In 0.3 there is a
HeapStats
struct and anAdHocStats
struct. There areHeapStats::get()
andAdHocStats::get()
functions for getting stats. The field names all make sense. The fields in these structs are all public, which is arguably bad, but changes are unlikely because that would also require changes to DHAT in Valgrind and the DHAT viewer, and the file format shared by all.ProfilerBuilder
There are now more configurations for a
Profiler
, soProfilerBuilder
exists to specify these. (You can useProfiler::new_{heap,ad_hoc}
for the most basic cases.)ProfilerBuilder
is a pretty standard use of the builder pattern. Some of the methods are argument-free and set a flag internally, e.g.ad_hoc()
andtesting()
. One might argue that they should have arguments, something like:kind(ProfilerKind::AdHoc)
instead ofad_hoc()
testing(true)
instead oftesting()
But the alternatives (
mode(ProfilerKind::Heap)
andtesting(false)
) would never occur in practice because they are the defaults. So it would be unnecessary generality that hurts conciseness.There are also a couple of hidden methods that are only useful for testing purposes.
Heap usage testing
0.2 could be used in one way: for profiling.
0.3 adds a new use: heap usage testing. You can write a test for your library crate that executes some code, and then write assertions about how much memory was allocated, such as exactly M allocations occurred, or fewer than N bytes were allocated. (Ad hoc usage testing is also possible, but it's less likely to be used.)
Integration tests must be used, with one test per file. This is because there is global state involved, and multiple tests in the same process would interfere with each other. Testing mode must be turned on via
ProfilerBuilder::testing()
. If that's done, the following occurs.Profiler
drop is disabled.dhat::assert!
,dhat::assert_eq!
, anddhat::assert_ne!
become available for use. (They panic immediately if used while not in testing mode.)An example, which should be put within
tests/
:Custom assertions are required because they behave differently to
std
assertions: on failure, they save data to file before panicking with an error message. The newly established preference for avoidinguse
statements fordhat
identifiers means that these will be explicitly qualified, and are unlikely to be confused with thestd
equivalents.Panics
0.3 panics in a number of situations that 0.2 did not.
Profiler
is created. (Previously it was ignored.)Profiler
isn't running. (Previously it was ignored.)dhat
assertion is called while aProfiler
isn't running, or if the runningProfiler
isn't in test mode. (These scenarios didn't exist in 0.2.)These are scenarios that only occur if the
dhat
user has done something wrong, and panicking is generally easier—either fordhat
itself, or for the user, or both—than trying to handle it gracefully (e.g. ignoring actions, or return anOption
).Backtrace trimming
0.2 would always get full length backtraces. These can be large, e.g. 100+ frames in big programs. Processing these causes a lot of overhead.
0.3 limits the length of backtraces. By default it's 10 "real" frames, but once inline frames get added it's often 20 or more. This is almost always more than enough to work out what's going on. The
ProfilerBuilder::trim()
method can be used to adjust the length limit, and "unlimited" is a possibility.It would be better if the length limit was precise, but the getting of the real frames happens when the raw backtrace is obtained, and the inline frame addition happens later when aggregated backtraces are "resolved" using debug info. If the backtraces were resolved upfront the lengths could be precise, but this would greatly increase the amount of resolving that happens, which is very expensive.
There's also an improved mechanism for trimming uninteresting frames at the top and bottom of backtraces. (This is separate to the length limit, and these trimmed frames don't count towards the limit.) This can also be turned off with
ProfilerBuilder::trim()
, though it's unlikely to be of interest except fordhat
testing/development purposes.Conclusion
I think this is a solid re-design of the APIs, and the heap usage testing is a genuinely useful capability that isn't available in any other way.
I would like to hear feedback about the APIs and the implementation. Reports from people who have tried it out on real-world code are particularly appreciated. Thanks!
Once a certain amount of time has passed (probably 2 weeks) I will make adjustments based on feedback. If there aren't any major issues I will do a 0.3.0 release shortly after that.
The text was updated successfully, but these errors were encountered: