[PATCH] D39086: Performance tracing facility for clangd.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Trace.cpp:87
+
+static Tracer* T = nullptr;
+} // namespace
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Maybe use `unique_ptr<Tracer>` (or even `llvm::Optional<Tracer>`)? Otherwise we leak memory and don't flush the stream if users of tracing API forget to call `stop()`. (I think I'm on the same page with @ioeric here  about forgetting a call to `stop()`).
> >  
> > Having a static global of non-pod type is a no-go, but we could use static local variables:
> > 
> > ```
> > llvm::Optional<Tracer>& getTracerInstance() {
> >  static llvm::Optional<Tracer> Tracer;
> >  return Tracer;
> > }
> > ```
> So Eric's solution of a scoped object that would be owned by main() makes sense to me, and I'm happy to add it (though personally not sure it carries its weight).
> 
> However I don't understand *your* suggestion :-)
> > Having a static global of non-pod type is a no-go, but we could use static local variables:
> >     llvm::Optional<Tracer>& getTracerInstance() {
> >       static llvm::Optional<Tracer> Tracer;
> >       return Tracer;
> >     }
> So it's not global, which means we're not calling Optional() on startup, which would actually be totally safe (I guess the constructor should be marked as `constexpr`).
> 
> But we *are* calling `~Optional()` and therefore `~Tracer()` at shutdown. And maybe `~Tracer()` calls something that logs, and llvm::errs() has already been destroyed... *this* is why avoid static objects, right?
> 
> > Otherwise we leak memory and don't flush the stream if users of tracing API forget to call stop()
> The intended time to call `stop()` is at the end of the program. If you forget to call it, we won't be leaking memory :-) We indeed won't flush the stream, but that wouldn't be safe (see above).
> 
> The other option is the user is calling `start()` and `stop()` multiple times, and they call `start()` twice without stopping. We already assert on this, and anyone who doesn't reproducibly hit that assert is well into racy UB-land (logging methods may be called concurrently with each other, but everything else must be synchronized).
> 
> Overall I'm not really seeing what scenario we can detect and safely do something about.
Forget what I said, you're right. Tracer has state, can be reconfigured multiple times and there's no guarantee our references won't be dangling on destruction of `Tracer`.

I do think Eric's solution is good, though. For all the same reasons why RAII is good.


https://reviews.llvm.org/D39086