jvm-profiling-tools / honest-profiler

A sampling JVM profiler without the safepoint sample bias
https://github.com/RichardWarburton/honest-profiler/wiki
MIT License
1.25k stars 146 forks source link

agent: Refactor memory management in Profiler class #228

Closed ikavalio closed 6 years ago

ikavalio commented 6 years ago

Hi,

This PR is big, but it doesn't change any existing functionality. It aims to simplify memory management in Profiler (no need to delete things explicitly) and enable profiler configuration test in Travis build (currently it creates a JVM, though technically tests don't need it and ASAN doesn't like it).

Summary of important changes:

  1. Replace all char* in ConfigurationOptions with std::string. This allows not to use safe_copy_string and safe_free_string functions. Though strings are copied rather than just assigned as char* it doesn't affect profiler's runtime (time when profiler is actually running), but makes code much simpler.
  2. Processor owns CircularQueue and SignalHandler. Processor will manage both buffer and signal handler. Previously signal handling logic was distributed between Profiler and Processor, but it can be done just by Processor. Moreover lifetimes of both buffer and handler are now limited by lifetime of processor.
  3. Use std::unique_ptr to manage references to LogWriter and Processor instead of using pointers directly. There's no need to write if's/delete's and explicit destructor.
  4. LogWriter now owns both ostream and ofstream. It makes things much easier and allows implementation of e.g. log rotation just within LogWriter. Old interface is still available for testing purposes. I also moved Profiler::lookupFrameInformation to LogWriter since there's no need for it to access Profiler specific things.
  5. Split Profiler::handle logic between Profiler::handle and Processor::handle. Profiler has access to JVM and gets a JNI env, but Processor does actual AGCT and pushes info to buffer. This allows to use Profiler in tests w/o actual JVM.

All changes are GCC 4.4 compatible.

RichardWarburton commented 6 years ago

Thanks again for the PR @ikavalio - great stuff. I've just put a couple of questions around the cleaning up of objects before merging.

ikavalio commented 6 years ago

Hi @RichardWarburton, thanks for your comments. In both cases everything should be handled correctly and I don't see any errors reported by address/leak sanitizer.

All std::string fields of ConfigurationOptions will copy content (in some cases move) from source strings no matter if they are raw char* (in agent.cpp copying std::string::assign is used) or other std::string, so destructor/assignment operator will free occupied memory correctly and transparently. That's why I'm using std::string, so we don't need to keep track of the owner of char*.

std::array in this case is identical to int timingIntervals[NUMBER_OF_INTERVALS]; since NUMBER_OF_INTERVALS is a compile time constant and it will be freed automatically when new Processor is created (e.g. if user changes sampling interval settings) and current goes out of the scope.

RichardWarburton commented 6 years ago

Gotcha, thanks.