[llvm] r229421 - Bitcode: Fix major regression: large files w/ debug info

[Hans Wennborg]

Merged in r229545.

Thanks,
Hans

On Mon, Feb 16, 2015 at 12:32 PM, Eric Christopher <echristo at gmail.com> wrote:
> FWIW this seems like a good idea to me (and you actually forgot the CC
> afaict).
>
> -eric
>
> On Mon Feb 16 2015 at 11:30:58 AM Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>>
>> Hans,
>>
>> If it's at all possible, this commit should be pulled into 3.6.x
>> before you release 3.6.0.
>>
>> By now I've rewritten all the code for bitcode reading/writing of
>> metadata, so you probably just need my approval.  I've also CC'ed
>> Joe Abbey, who is listed in CODE_OWNERS.txt, in case you want his
>> approval as well.
>>
>> Sorry for the late patch...
>>
>> > On 2015-Feb-16, at 11:18, Duncan P. N. Exon Smith <dexonsmith at apple.com>
>> > wrote:
>> >
>> > Author: dexonsmith
>> > Date: Mon Feb 16 13:18:01 2015
>> > New Revision: 229421
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=229421&view=rev
>> > Log:
>> > Bitcode: Fix major regression: large files w/ debug info
>> >
>> > The metadata/value split introduced a major regression reading large
>> > bitcode files that contain debug info (or other cyclic (non-self
>> > reference) metadata graphs).  For the first time in a while, I dropped
>> > from libLTO.dylib down to `llvm-lto` with a non-trivial bitcode file
>> > (~350MB), and I hit this when reading the result of ld64's `-save-temps`
>> > in `llvm-lto`.
>> >
>> > Here's pseudo-code for what was going on:
>> >
>> >    read-main-metadata-block:
>> >      for each md:
>> >        if has-fwd-ref: // Only true for cyclic graphs.
>> >          any-fwd-refs <- true
>> >      if any-fwd-refs:
>> >        foreach md:
>> >          resolve-cycles(md) // Handle cycles.
>> >
>> >    foreach function:
>> >      read-function-metadata-block: // Such as !alias, !loop
>> >        if any-fwd-refs:
>> >          foreach md: // (all metadata, not just this block)
>> >            resolve-cycles(md) // A no-op, but the loop is expensive!!
>> >
>> > This commit resets the `AnyFwdRefs` flag to `false`.  This on its own
>> > was enough to change my Release+Asserts `llvm-lto` time for reading this
>> > bitcode from over 20 minutes (I gave up on it) to 20 seconds.  I've gone
>> > further by tracking the min/max metadata forward-references in a
>> > metadata block.  This protects against a schema that has lots of
>> > functions that each reference their own metadata cycle.
>> >
>> > Unfortunately, this regression is in the 3.6 branch as well.
>> >
>> > Modified:
>> >    llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>> >    llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h
>> >
>> > Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=229421&r1=229420&r2=229421&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
>> > +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Mon Feb 16 13:18:01
>> > 2015
>> > @@ -555,9 +555,17 @@ Metadata *BitcodeReaderMDValueList::getV
>> >   if (Metadata *MD = MDValuePtrs[Idx])
>> >     return MD;
>> >
>> > -  // Create and return a placeholder, which will later be RAUW'd.
>> > -  AnyFwdRefs = true;
>> > +  // Track forward refs to be resolved later.
>> > +  if (AnyFwdRefs) {
>> > +    MinFwdRef = std::min(MinFwdRef, Idx);
>> > +    MaxFwdRef = std::max(MaxFwdRef, Idx);
>> > +  } else {
>> > +    AnyFwdRefs = true;
>> > +    MinFwdRef = MaxFwdRef = Idx;
>> > +  }
>> >   ++NumFwdRefs;
>> > +
>> > +  // Create and return a placeholder, which will later be RAUW'd.
>> >   Metadata *MD = MDNode::getTemporary(Context, None).release();
>> >   MDValuePtrs[Idx].reset(MD);
>> >   return MD;
>> > @@ -573,7 +581,8 @@ void BitcodeReaderMDValueList::tryToReso
>> >     return;
>> >
>> >   // Resolve any cycles.
>> > -  for (auto &MD : MDValuePtrs) {
>> > +  for (unsigned I = MinFwdRef, E = MaxFwdRef + 1; I != E; ++I) {
>> > +    auto &MD = MDValuePtrs[I];
>> >     auto *N = dyn_cast_or_null<MDNode>(MD);
>> >     if (!N)
>> >       continue;
>> > @@ -581,6 +590,9 @@ void BitcodeReaderMDValueList::tryToReso
>> >     assert(!N->isTemporary() && "Unexpected forward reference");
>> >     N->resolveCycles();
>> >   }
>> > +
>> > +  // Make sure we return early again until there's another forward ref.
>> > +  AnyFwdRefs = false;
>> > }
>> >
>> > Type *BitcodeReader::getTypeByID(unsigned ID) {
>> >
>> > Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h?rev=229421&r1=229420&r2=229421&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h (original)
>> > +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.h Mon Feb 16 13:18:01
>> > 2015
>> > @@ -99,6 +99,8 @@ public:
>> > class BitcodeReaderMDValueList {
>> >   unsigned NumFwdRefs;
>> >   bool AnyFwdRefs;
>> > +  unsigned MinFwdRef;
>> > +  unsigned MaxFwdRef;
>> >   std::vector<TrackingMDRef> MDValuePtrs;
>> >
>> >   LLVMContext &Context;
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits