[PATCH] D118677: [llvm-profgen] Clean up unnecessary memory reservations between phases.

Hongtao Yu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Feb 1 12:11:03 PST 2022


hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:161
   Generator->generateProfile();
+  Reader.release();
   Generator->write();
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > wenlei wrote:
> > > > > nit: using a lexical scope for this purpose might be better. 
> > > > Lexical scope is nice, but in this case `Reader` and `Generator` have overlapping lifetimes, i.e, we need `Reader` to be released before `Generator->write();` but after `Generator->generateProfile();`. So one lexical scope including both objects may not work.
> > > > 
> > > > Actually maybe we can define `Reader->getSampleCounters()` as a `unique_ptr` too, so that when it is transferred to `Generator`, `Generator` will take its ownership and release it when appropriate.
> > > > Lexical scope is nice, but in this case Reader and Generator have overlapping lifetimes, i.e, we need Reader to be released before Generator->write(); but after Generator->generateProfile();. So one lexical scope including both objects may not work. 
> > > 
> > > I was thinking about hoisting the unique_ptr for Generator above, initialized with null, then set to the real thing after reader is available. This way the lexical scope only applies to reader.
> > > 
> > > > Actually maybe we can define Reader->getSampleCounters() as a unique_ptr too, so that when it is transferred to Generator, Generator will take its ownership and release it when appropriate. 
> > > 
> > > I'm not sure if this is a good idea. With this, we can leave Reader in a corrupted state after calling the getter, which is probably not a good API design. 
> > > 
> > > 
> > 
> > So far Reader is released between two Generator invocations. This relies on the knowledge that Reader is only used in first invocation. I was thinking that Reader could be released immediately after the needed data is passed into Generator. Usually this would be done by passing SampleCounters by value from Reader into Generator. We are now passing it by reference which holds reader live until Generator doesn't use it. This seems a tangled definition without a clear boundary between the two objects. Maybe defining SampleCounters as a shared pointer is better?
> > 
> > 
> > > I was thinking about hoisting the unique_ptr for Generator above, initialized with null, then set to the real thing after reader is available. This way the lexical scope only applies to reader.
> > 
> > Yeah, this way should work. The adjusted code look like below. Still feels a bit weird. Maybe just make the release call explicitly if we don't want to change the api?
> > 
> > 
> > ```
> > PerfInputFile PerfFile = getPerfInputFile();
> > std::unique_ptr<ProfileGeneratorBase> Generator;
> > 
> > {
> >     std::unique_ptr<PerfReaderBase> Reader =
> >         PerfReaderBase::create(Binary.get(), PerfFile);
> >     // Parse perf events and samples
> >     Reader->parsePerfTraces();
> > 
> >     if (SkipSymbolization)
> >       return EXIT_SUCCESS;
> > 
> >     Generator =
> >       ProfileGeneratorBase::create(Binary.get(), Reader->getSampleCounters(),
> >                                    Reader->profileIsCSFlat());
> >     Generator->generateProfile();
> >   }
> > 
> >   Generator->write();
> > ```
> I don't have strong opinion. It's ok to leave it as is, but add a comment to explain why the explicit release/scope is needed. 
> 
> > Still feels a bit weird. 
> 
> It is just as weird as a release in the middle. :) 
> It is just as weird as a release in the middle. :)

Agreed. I hope we'll have some time to readdress the api design in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118677/new/

https://reviews.llvm.org/D118677



More information about the llvm-commits mailing list