Right now the profiler manages ProfilerBacktrace instances like this: (A) "Code A" calls profiler_get_backtrace() and sticks the result in a raw ProfilerBacktrace pointer. (B) Later on, "Code B" calls profiler_tracing() and passes in that raw pointer. We *must* now null out the raw pointer, because profiler_tracing is now implicitly taking ownership. (C) We also need "Code C" cleanup-code, to call profiler_free_backtrace() on the raw pointer, in case "Code B" was never hit. This is complex, and it's hard to sanity-check that the pointer is being managed correctly here. Example scenarios: - in current mozilla-central, we have raw-pointers mStyleCause & mReflowCause. "Code A" is AddStyleFlushObserver / AddReflowFlushObserver (in RefreshDriver.h). "Code B" is a deeply-nested conditional in nsRefreshDriver::Tick(). "Code C" is ~RefreshDriver - In attachment 8575801 [details] [diff] [review] for bug 1129249, "Code A" is RestyleTracker::AddPendingRestyleToTable in RestyleTracker.h. "Code B" is ElementRestyler::ComputeStyleChangeFor() in RestyleManager.cpp. And right now there is no "Code C" for this example, which may be a bug. (I think it's because of an implicit assumption that Code B must always be hit, but I'm not sure yet if that's valid or not.) If we used UniquePtr instead of raw pointers here, then we'd get the following benefits: - Ownership transfers would be immediate & enforced by the type system. (No need for any explicit nulling out in "Code B"; we can make that happen automatically when we pass in our UniquePtr to profiler_free_backtrace().) - Cleanup would happen automatically. (No need to worry about "Code C" at all.) - As a result, it's easier to skim the code & convince yourself it's correct. Any thoughts/objections?
No objections at all.
Note that UniquePtr uses "delete" by default, whereas we seem to clean up ProfilerBacktrace instances via "profiler_free_backtrace" (which is really just 'delete' under the hood). We have the option of using the second UniquePtr template-argument to provide a custom deletion policy (using profiler_free_backtrace), though if possible, my preference would be to just do away with profiler_free_backtrace and just let UniquePtr call "delete" directly. (AFAICT, the only reason we have profiler_free_backtrace is so we can make it a no-op in builds that have profiling disabled. This doesn't seem to be a big benefit, though, because "delete myNullPtr" is also practically a no-op. (just a null-check, presumably).)
I'm the original author of the profiler_*_backtrace() functions. I support the conversion to UniquePtr. Also, the profile_free_backtrace was essentially there to enforce a convention allocation and deallocation be abstracted away. OTOH by returning a UniquePtr from profiler_get_backtrace, the deallocation mechanism is already encapsulated. r+ from me to eliminate profiler_free_backtrace when converting to UniquePtr.
Great! (& thanks for the backstory! I was worried there might be a deeper reason we need to keep profiler_free_backtrace(), but I'm glad that's not the case.)
I probably don't have cycles to jump on this at the moment; just wanted to make sure it was a sane idea & that it's on the to-do list. If anyone else wants to take this, feel free!
I have a patch for this.
Created attachment 8823392 [details] [diff] [review] manage ProfilerBacktrace with UniquePtr This patch applies (for me) on top of the patches in bug 1322863; it's possible that this patch builds just fine without the other ones, but I have not tested it.
Comment on attachment 8823392 [details] [diff] [review] manage ProfilerBacktrace with UniquePtr Review of attachment 8823392 [details] [diff] [review]: ----------------------------------------------------------------- Since the destructor just calls delete anyway, is there a reason you're not just using UniquePtr<ProfilerBacktrace> and MakeUnique<ProfileBacktrace>(...)?
(In reply to Markus Stange [:mstange] from comment #8) > Since the destructor just calls delete anyway, is there a reason you're not > just using UniquePtr<ProfilerBacktrace> and > MakeUnique<ProfileBacktrace>(...)? I *think* the reason was that the definition of ProfilerBacktrace isn't necessarily visible in all the places that ProfilerBacktrace gets constructed, and calling |delete| on such instances doesn't work/undefined behavior/triggers warnings/etc. Doing things this way lets indirection take care of such problems for us. I am open to making things less complicated, though...
Comment on attachment 8823392 [details] [diff] [review] manage ProfilerBacktrace with UniquePtr Review of attachment 8823392 [details] [diff] [review]: ----------------------------------------------------------------- Ok, if that is the case, sure. This patch is an improvement either way.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/603bdc33779f manage ProfilerBacktrace with UniquePtr; r=mstange