David Blaikie via llvm-dev
2016-Apr-15 17:27 UTC
[llvm-dev] [RFC] Lazy-loading of debug info metadata
On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith < dexonsmith at apple.com> wrote:> Since I haven't heard any objections to the direction, I'm planning to > commit this (step 4) when I find some cycles; likely over the weekend. > > To make this more concrete (in case someone is silently concerned) I've > posted my WIP patches below. They apply cleanly to r266414. There are > a few mechanical changes missing that are tracked in the commit > messages (such as LangRef, or new tests in some cases). To get > `ninja check` to pass after the final commit you'll need to run the > attached upgrade script. The clang changes are truly uninteresting so > I've left them out. > > - 0001/2: Prep commits I need to flush out (sorry for the noise). > - 0003: Add an explicit type map for ODR type uniquing. > - 0004: Prep. > - 0005: Add ODR type uniquing of members. >What's this ^ for? In theory, the list of /members/ of a type shouldn't vary between instances. The special cases (implicit special members, member function template specializations, and nested types) don't appear in the member list - instead they just list the type as their scope (so it's one directional, instead of bidirectional) Is this to handle ODR violations? We currently don't handle them, right - we just pick one instance of the type & the user gets to keep the pieces. Is there a particular motivation for changing that? Some place where it's more problematic, we have to support it, etc?> - 0006: Remove DITypeRef (string-based references) and strip down > 'retainedTypes:'. > > Once this is done, I expect the bitcode block layout improvements for > lazy-loading of subprograms and composite types (steps 3 and 5 from > this RFC) to be fairly straightforward. > > > > > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at gmail.com> wrote: > > > > I have no objections to any of this FWIW :) > > > > -eric > > > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith < > dexonsmith at apple.com> wrote: > > > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo at gmail.com> wrote: > > > > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie <dblaikie at gmail.com> > wrote: > > >> +pcc, who had some other ideas/patch out for improving memory usage > of debug info > > >> +Reid, who's responsible for the windows/CodeView/PDB debug info > which is motivating some of the ideas about changes to type emission > > > > > > So I discussed this with Adrian and Mehdi at the social last Thursday > and I'm getting set to finish the write up. I think it'll have some bearing > on this proposal as I think it'll change how we want to take a look at the > format of DISubprogram metadata a bit more. > > > > (The interesting bit here is to make a clearer split between > > DISubprogram declarations (part of the type hierarchY) and > > DISubprogram definitions (part of the code/line table/variable > > locations). I think that'll end up being mostly orthogonal to what > > I'm trying to do.) > > > > > That said, most of it is orthogonal to the changes Duncan is talking > about here. Just puts the pressure on to get the other proposal written up. > > > > Which is now here: > > http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html > > > > >> Baking into the IR more about types as units has pretty direct > overlap with Reid/CodeView/etc - so, yeah, that'll takes ome discussion > (but, as you say, it's not in your immediate plan anyway, so we can come > back to that - but would be good for whoever gets there first to discuss it > with the others) > > > > After thinking for a few days, I don't think this will bake anything > > new into the IR. If anything it removes a few special cases. > > > > More at the bottom. > > > > >>> Motivation > > >>> =========> > >>> > > >>> Based on some analysis Mehdi ran (ping him for details), there are > three > > >>> (related) compile-time bottlenecks we're seeing with `-flto=thin -g`: > > >>> > > >>> a) Reading the large number of Metadata bitcode records in the > global > > >>> metadata block. I'm talking about raw `BitStreamer` calls here. > > >>> > > >>> b) Creating unnecessary `DI*` instances (that aren't relevant to > code). > > >>> > > >>> c) Emitting unnecessary `DI*` instances (that aren't relevant to > code). > > >>> > > >>> Here is my recollection of some peak memory stats on a small testcase > > >>> during thin-LTO, which should be a decent indicator of (b): > > >>> > > >>> - ~150MB: DILocation > > >>> - ~100MB: DISubprogram > > >>> - ~70MB: DILocalVariable > > >>> - ~50MB: (cumulative) DIType descendents > > >>> > > >>> It looks, suprisingly, like types are not the primary bottleneck. > > > > (Probably wrong for (a), BTW. Caveats matter.) > > > > >>> There are caveats: > > >>> > > >>> - `DISubprogram` declarations -- member function descriptors -- are > > >>> part of the type hierarchy. > > >>> - Most of the type hierarchy gets uniqued at parse time. > > >>> - As a result, these data are a poor indicator for (a). > > > > ((a) is the main bottleneck for compile-time of -flto=thin (since it's > > quadratic in the number of files). (b) only affects memory. Also > > important, but at least it scales linearly.) > > > > >>> Even so, non-types are substantial. > > >>> > > >>> Proposal > > >>> =======> > >>> > > >>> Short version > > >>> ------------- > > >>> > > >>> 4. Remove `DICompositeType`s from `retainedTypes:`, similar to (2). > > > > This is the part that's relevant to the new RFC Eric just posted. > > > > >>> Long version > > >>> ------------- > > >>> > > >>> 4. Implement my proposal to remove the `DICompositeType` name map > from > > >>> `retainedTypes:`. > > >>> > > >>> > http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html > > >>> > > >>> Similar to (2) above, this will naturally filter the types that > get > > >>> linked in to the ones actually used by the code being linked. > > >>> > > >>> It should also allow the reader to skip records for types that > have > > >>> already been loaded in the main module. > > > > The essential things I want to accomplish with this part: > > > > - Make `type:` operands less special: instead of referencing types > > indirectly through MDString, point directly at the type node. > > > > - Stop using `retainedTypes:` by default (only for -gfull, etc.). > > > > - Avoid blowing up memory in -flto=full (which converting MDString > > references back to pointers would do naively, through > > re-introducing cycles). Note that this needs to be handled > > somehow at BitcodeReader time. > > > > After chatting with Eric, I don't think this conflicts at all with the > > other RFC. Unifying the `type:` operands might actually help both. > > > > One good point David mentioned last week was that we don't want to > > teach the IR any more about types. Rather than inventing some new > > context (as I suggested originally), I figure LTO plugins can just > > pass a (StringRef -> DIType*) map to the BitcodeReader, and the Module > > itself doesn't need to know anything about it. > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160415/feed59d8/attachment.html>
Duncan P. N. Exon Smith via llvm-dev
2016-Apr-15 21:27 UTC
[llvm-dev] [RFC] Lazy-loading of debug info metadata
> On 2016-Apr-15, at 10:27, David Blaikie <dblaikie at gmail.com> wrote: > > > > On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > Since I haven't heard any objections to the direction, I'm planning to > commit this (step 4) when I find some cycles; likely over the weekend. > > To make this more concrete (in case someone is silently concerned) I've > posted my WIP patches below. They apply cleanly to r266414. There are > a few mechanical changes missing that are tracked in the commit > messages (such as LangRef, or new tests in some cases). To get > `ninja check` to pass after the final commit you'll need to run the > attached upgrade script. The clang changes are truly uninteresting so > I've left them out. > > - 0001/2: Prep commits I need to flush out (sorry for the noise). > - 0003: Add an explicit type map for ODR type uniquing. > - 0004: Prep. > - 0005: Add ODR type uniquing of members. > > What's this ^ for? > > In theory, the list of /members/ of a type shouldn't vary between instances. The special cases (implicit special members, member function template specializations, and nested types) don't appear in the member list - instead they just list the type as their scope (so it's one directional, instead of bidirectional) > > Is this to handle ODR violations? We currently don't handle them, right - we just pick one instance of the type & the user gets to keep the pieces. Is there a particular motivation for changing that? Some place where it's more problematic, we have to support it, etc?This is a strange one; I probably should have called it out actually. It's a bugfix vs. ToT; and without it, 0006 regresses DWARF output. File and line can change without violating ODR, which means that we end up with duplicate members. I think I may have written something up in the WIP commit message for 0005, but also have a look in tree at test/Linker/type-unique-odr-a.ll. It's legal, e.g., to have the following in two separate files, as long as the sequence of tokens is identical: -- struct S { void foo(); }; -- However, the DISubprogram(isDefinition: false) for S::foo will not be structurally equivalent (since the 'file:' node won't match), so we currently end up with two metadata nodes. The same thing happens for the DICompositeType for `S` (and also applies to any fields). The checked-in testcase confirms that the description of `S` only has one copy of `S::foo` in the DWARF output. (The testcase doesn't provide good enough coverage actually; it gets lucky because the definition of S::foo is in the second file being linked (type-unique-odr-b.ll). If you reverse the order of the files, then `S` will have two copies of `S::foo`.) 0005 solves the problem up front (and in both directions; I'll add a RUN line to test/Linker/type-unique-odr-a.ll). I came across this because the testcase started failing with 0006. The current (but not-quite-sufficient) logic relies on the resolve() logic in DwarfDebug.cpp, which indirects through the merged 'retainedTypes:' map (the asymmetry in building the merged map explains why the in-tree logic only works for one linking direction).> > - 0006: Remove DITypeRef (string-based references) and strip down > 'retainedTypes:'. > > Once this is done, I expect the bitcode block layout improvements for > lazy-loading of subprograms and composite types (steps 3 and 5 from > this RFC) to be fairly straightforward. > > > > > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at gmail.com> wrote: > > > > I have no objections to any of this FWIW :) > > > > -eric > > > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > > > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo at gmail.com> wrote: > > > > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie <dblaikie at gmail.com> wrote: > > >> +pcc, who had some other ideas/patch out for improving memory usage of debug info > > >> +Reid, who's responsible for the windows/CodeView/PDB debug info which is motivating some of the ideas about changes to type emission > > > > > > So I discussed this with Adrian and Mehdi at the social last Thursday and I'm getting set to finish the write up. I think it'll have some bearing on this proposal as I think it'll change how we want to take a look at the format of DISubprogram metadata a bit more. > > > > (The interesting bit here is to make a clearer split between > > DISubprogram declarations (part of the type hierarchY) and > > DISubprogram definitions (part of the code/line table/variable > > locations). I think that'll end up being mostly orthogonal to what > > I'm trying to do.) > > > > > That said, most of it is orthogonal to the changes Duncan is talking about here. Just puts the pressure on to get the other proposal written up. > > > > Which is now here: > > http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html > > > > >> Baking into the IR more about types as units has pretty direct overlap with Reid/CodeView/etc - so, yeah, that'll takes ome discussion (but, as you say, it's not in your immediate plan anyway, so we can come back to that - but would be good for whoever gets there first to discuss it with the others) > > > > After thinking for a few days, I don't think this will bake anything > > new into the IR. If anything it removes a few special cases. > > > > More at the bottom. > > > > >>> Motivation > > >>> =========> > >>> > > >>> Based on some analysis Mehdi ran (ping him for details), there are three > > >>> (related) compile-time bottlenecks we're seeing with `-flto=thin -g`: > > >>> > > >>> a) Reading the large number of Metadata bitcode records in the global > > >>> metadata block. I'm talking about raw `BitStreamer` calls here. > > >>> > > >>> b) Creating unnecessary `DI*` instances (that aren't relevant to code). > > >>> > > >>> c) Emitting unnecessary `DI*` instances (that aren't relevant to code). > > >>> > > >>> Here is my recollection of some peak memory stats on a small testcase > > >>> during thin-LTO, which should be a decent indicator of (b): > > >>> > > >>> - ~150MB: DILocation > > >>> - ~100MB: DISubprogram > > >>> - ~70MB: DILocalVariable > > >>> - ~50MB: (cumulative) DIType descendents > > >>> > > >>> It looks, suprisingly, like types are not the primary bottleneck. > > > > (Probably wrong for (a), BTW. Caveats matter.) > > > > >>> There are caveats: > > >>> > > >>> - `DISubprogram` declarations -- member function descriptors -- are > > >>> part of the type hierarchy. > > >>> - Most of the type hierarchy gets uniqued at parse time. > > >>> - As a result, these data are a poor indicator for (a). > > > > ((a) is the main bottleneck for compile-time of -flto=thin (since it's > > quadratic in the number of files). (b) only affects memory. Also > > important, but at least it scales linearly.) > > > > >>> Even so, non-types are substantial. > > >>> > > >>> Proposal > > >>> =======> > >>> > > >>> Short version > > >>> ------------- > > >>> > > >>> 4. Remove `DICompositeType`s from `retainedTypes:`, similar to (2). > > > > This is the part that's relevant to the new RFC Eric just posted. > > > > >>> Long version > > >>> ------------- > > >>> > > >>> 4. Implement my proposal to remove the `DICompositeType` name map from > > >>> `retainedTypes:`. > > >>> > > >>> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html > > >>> > > >>> Similar to (2) above, this will naturally filter the types that get > > >>> linked in to the ones actually used by the code being linked. > > >>> > > >>> It should also allow the reader to skip records for types that have > > >>> already been loaded in the main module. > > > > The essential things I want to accomplish with this part: > > > > - Make `type:` operands less special: instead of referencing types > > indirectly through MDString, point directly at the type node. > > > > - Stop using `retainedTypes:` by default (only for -gfull, etc.). > > > > - Avoid blowing up memory in -flto=full (which converting MDString > > references back to pointers would do naively, through > > re-introducing cycles). Note that this needs to be handled > > somehow at BitcodeReader time. > > > > After chatting with Eric, I don't think this conflicts at all with the > > other RFC. Unifying the `type:` operands might actually help both. > > > > One good point David mentioned last week was that we don't want to > > teach the IR any more about types. Rather than inventing some new > > context (as I suggested originally), I figure LTO plugins can just > > pass a (StringRef -> DIType*) map to the BitcodeReader, and the Module > > itself doesn't need to know anything about it.
David Blaikie via llvm-dev
2016-Apr-15 21:53 UTC
[llvm-dev] [RFC] Lazy-loading of debug info metadata
On Fri, Apr 15, 2016 at 2:27 PM, Duncan P. N. Exon Smith < dexonsmith at apple.com> wrote:> > > On 2016-Apr-15, at 10:27, David Blaikie <dblaikie at gmail.com> wrote: > > > > > > > > On Fri, Apr 15, 2016 at 9:50 AM, Duncan P. N. Exon Smith < > dexonsmith at apple.com> wrote: > > Since I haven't heard any objections to the direction, I'm planning to > > commit this (step 4) when I find some cycles; likely over the weekend. > > > > To make this more concrete (in case someone is silently concerned) I've > > posted my WIP patches below. They apply cleanly to r266414. There are > > a few mechanical changes missing that are tracked in the commit > > messages (such as LangRef, or new tests in some cases). To get > > `ninja check` to pass after the final commit you'll need to run the > > attached upgrade script. The clang changes are truly uninteresting so > > I've left them out. > > > > - 0001/2: Prep commits I need to flush out (sorry for the noise). > > - 0003: Add an explicit type map for ODR type uniquing. > > - 0004: Prep. > > - 0005: Add ODR type uniquing of members. > > > > What's this ^ for? > > > > In theory, the list of /members/ of a type shouldn't vary between > instances. The special cases (implicit special members, member function > template specializations, and nested types) don't appear in the member list > - instead they just list the type as their scope (so it's one directional, > instead of bidirectional) > > > > Is this to handle ODR violations? We currently don't handle them, right > - we just pick one instance of the type & the user gets to keep the pieces. > Is there a particular motivation for changing that? Some place where it's > more problematic, we have to support it, etc? > > This is a strange one; I probably should have called it out actually. > It's a bugfix vs. ToT; and without it, 0006 regresses DWARF output. > > File and line can change without violating ODR, which means that we > end up with duplicate members. I think I may have written something > up in the WIP commit message for 0005, but also have a look in tree at > test/Linker/type-unique-odr-a.ll. > > It's legal, e.g., to have the following in two separate files, as long > as the sequence of tokens is identical: > -- > struct S { void foo(); }; > -- > However, the DISubprogram(isDefinition: false) for S::foo will not be > structurally equivalent (since the 'file:' node won't match), so we > currently end up with two metadata nodes. The same thing happens for > the DICompositeType for `S` (and also applies to any fields). > > The checked-in testcase confirms that the description of `S` only has > one copy of `S::foo` in the DWARF output. (The testcase doesn't provide > good enough coverage actually; it gets lucky because the definition of > S::foo is in the second file being linked (type-unique-odr-b.ll). If > you reverse the order of the files, then `S` will have two copies of > `S::foo`.) > > 0005 solves the problem up front (and in both directions; I'll add a RUN > line to test/Linker/type-unique-odr-a.ll). > > I came across this because the testcase started failing with 0006. The > current (but not-quite-sufficient) logic relies on the resolve() logic > in DwarfDebug.cpp, which indirects through the merged 'retainedTypes:' > map (the asymmetry in building the merged map explains why the in-tree > logic only works for one linking direction). >Not sure I'm quite following. I think I understand the circumstance you're describing (indeed, the type itself can be defined on different lines/files without violating the ODR, or its members, or, etc... ). But if the subprogram declaration is in a different file in one definition - that'd make the composite type itself different (metadata uniquing - composite type references the subprogram, etc). But we should only every find one version of the type (all references to the type should be via string-based dityperefs). Sure, we'll pick one at random, but when/how would we get two?> > > > > - 0006: Remove DITypeRef (string-based references) and strip down > > 'retainedTypes:'. > > > > Once this is done, I expect the bitcode block layout improvements for > > lazy-loading of subprograms and composite types (steps 3 and 5 from > > this RFC) to be fairly straightforward. > > > > > > > > > > > On 2016-Mar-29, at 19:11, Eric Christopher <echristo at gmail.com> wrote: > > > > > > I have no objections to any of this FWIW :) > > > > > > -eric > > > > > > On Tue, Mar 29, 2016 at 6:46 PM Duncan P. N. Exon Smith < > dexonsmith at apple.com> wrote: > > > > > > > On 2016-Mar-22, at 20:11, Eric Christopher <echristo at gmail.com> > wrote: > > > > > > > >> On Tue, Mar 22, 2016 at 8:04 PM David Blaikie <dblaikie at gmail.com> > wrote: > > > >> +pcc, who had some other ideas/patch out for improving memory usage > of debug info > > > >> +Reid, who's responsible for the windows/CodeView/PDB debug info > which is motivating some of the ideas about changes to type emission > > > > > > > > So I discussed this with Adrian and Mehdi at the social last > Thursday and I'm getting set to finish the write up. I think it'll have > some bearing on this proposal as I think it'll change how we want to take a > look at the format of DISubprogram metadata a bit more. > > > > > > (The interesting bit here is to make a clearer split between > > > DISubprogram declarations (part of the type hierarchY) and > > > DISubprogram definitions (part of the code/line table/variable > > > locations). I think that'll end up being mostly orthogonal to what > > > I'm trying to do.) > > > > > > > That said, most of it is orthogonal to the changes Duncan is talking > about here. Just puts the pressure on to get the other proposal written up. > > > > > > Which is now here: > > > http://lists.llvm.org/pipermail/llvm-dev/2016-March/097773.html > > > > > > >> Baking into the IR more about types as units has pretty direct > overlap with Reid/CodeView/etc - so, yeah, that'll takes ome discussion > (but, as you say, it's not in your immediate plan anyway, so we can come > back to that - but would be good for whoever gets there first to discuss it > with the others) > > > > > > After thinking for a few days, I don't think this will bake anything > > > new into the IR. If anything it removes a few special cases. > > > > > > More at the bottom. > > > > > > >>> Motivation > > > >>> =========> > > >>> > > > >>> Based on some analysis Mehdi ran (ping him for details), there are > three > > > >>> (related) compile-time bottlenecks we're seeing with `-flto=thin > -g`: > > > >>> > > > >>> a) Reading the large number of Metadata bitcode records in the > global > > > >>> metadata block. I'm talking about raw `BitStreamer` calls > here. > > > >>> > > > >>> b) Creating unnecessary `DI*` instances (that aren't relevant to > code). > > > >>> > > > >>> c) Emitting unnecessary `DI*` instances (that aren't relevant to > code). > > > >>> > > > >>> Here is my recollection of some peak memory stats on a small > testcase > > > >>> during thin-LTO, which should be a decent indicator of (b): > > > >>> > > > >>> - ~150MB: DILocation > > > >>> - ~100MB: DISubprogram > > > >>> - ~70MB: DILocalVariable > > > >>> - ~50MB: (cumulative) DIType descendents > > > >>> > > > >>> It looks, suprisingly, like types are not the primary bottleneck. > > > > > > (Probably wrong for (a), BTW. Caveats matter.) > > > > > > >>> There are caveats: > > > >>> > > > >>> - `DISubprogram` declarations -- member function descriptors -- > are > > > >>> part of the type hierarchy. > > > >>> - Most of the type hierarchy gets uniqued at parse time. > > > >>> - As a result, these data are a poor indicator for (a). > > > > > > ((a) is the main bottleneck for compile-time of -flto=thin (since it's > > > quadratic in the number of files). (b) only affects memory. Also > > > important, but at least it scales linearly.) > > > > > > >>> Even so, non-types are substantial. > > > >>> > > > >>> Proposal > > > >>> =======> > > >>> > > > >>> Short version > > > >>> ------------- > > > >>> > > > >>> 4. Remove `DICompositeType`s from `retainedTypes:`, similar to > (2). > > > > > > This is the part that's relevant to the new RFC Eric just posted. > > > > > > >>> Long version > > > >>> ------------- > > > >>> > > > >>> 4. Implement my proposal to remove the `DICompositeType` name map > from > > > >>> `retainedTypes:`. > > > >>> > > > >>> > http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160125/327936.html > > > >>> > > > >>> Similar to (2) above, this will naturally filter the types > that get > > > >>> linked in to the ones actually used by the code being linked. > > > >>> > > > >>> It should also allow the reader to skip records for types that > have > > > >>> already been loaded in the main module. > > > > > > The essential things I want to accomplish with this part: > > > > > > - Make `type:` operands less special: instead of referencing types > > > indirectly through MDString, point directly at the type node. > > > > > > - Stop using `retainedTypes:` by default (only for -gfull, etc.). > > > > > > - Avoid blowing up memory in -flto=full (which converting MDString > > > references back to pointers would do naively, through > > > re-introducing cycles). Note that this needs to be handled > > > somehow at BitcodeReader time. > > > > > > After chatting with Eric, I don't think this conflicts at all with the > > > other RFC. Unifying the `type:` operands might actually help both. > > > > > > One good point David mentioned last week was that we don't want to > > > teach the IR any more about types. Rather than inventing some new > > > context (as I suggested originally), I figure LTO plugins can just > > > pass a (StringRef -> DIType*) map to the BitcodeReader, and the Module > > > itself doesn't need to know anything about it. > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160415/c74de578/attachment.html>