Slab allocator (for example Support\RecyclingAllocator.h) will be fast enough and would work certainly for fixed size classes. However not only Decls are dynamically allocated but a large number of other classes, this startegy will end up with tens of fixed size allocators in addition to a variable-sized catch-all for the less popular sizes and dynamic-sized allocations. Some examples:
NamedDecl **NamedChain = new (SemaRef.Context)NamedDecl*[Chaining.size()]; TemplateArgument *ArgumentPack = new (S.Context) TemplateArgument[Pack.New.size()]; Since the recycled memory is not shared between the different-sized fixed and the dynamic allocators there will be significant waste. Not all allocations go through ASTConext. Search for '.Allocate<' to find some that use the allocator directly. Not all allocs are paired with a free. That could be found using the regular tools, if taught to watch for it. Revising clang memory management is quite a task, while keeping top performance. It does have the potential to lower clang peak memory usage while compiling big programs. There is an excellent discussion of small object allocation in Chapter 4 of "Modern C++ Design: Generic Programming and Design Patterns Applied" / Andrei Alexandrescu. Yaron 2014-08-04 13:42 GMT+03:00 Vassil Vassilev <[email protected]>: > Hi Yaron, > Thanks for raising those very interesting points! > > On 08/04/2014 12:12 PM, Yaron Keren wrote: > > Hi, > > You should not destruct SmallVector explicitly as its destructor will be > called from ~MacroInfo from ~MacroInfoChain from ~Preprocessor. If you want > to reclaim the Tokens memory use in MacroInfo use you need to do as before, > call ~MacroInfo and remove it from the chain. > > Understood. > > > The source of the memory usage comes much more from AST memory "leakage" > (leakage in the sense the memory is not freed until destruction which does > not happen in cling) and other allocations all around the code rather than > the bit of memory lost to the macros. > > Yes, I was planning to replace the bump alloc with a slab alloc and > measure the performance penalty. This is on my todo list since some time. > What would be your guess? > > > I have looked into this issue for Ceemple which has similar need as > cling for memory reclaim and gave it up for now. > It's actually quite hard to make clang reclaim memory before destruction, > since > > I couldn't agree more. > > > 1) BumpPtrAllocator does not reuse free memory. Could be replaced by a > MallocAllocator or other custom allocation but this would reduce > compilation performance. It's hard to compete with BumpPtrAllocator > performance. > > I think using a slab alloc may be not that bad for us. > > > 2) Freeing the memory slows down performance even more. BumpPtrAllocator > free is a no-op. > > For our use-cases this is not an issue. This is on the error path, there > we can be slower. Memory is much much more important. > > > 3) Actually releasing the memory may cause use-after-free bugs which are > not seen now since the AST memory is never really released. > > Another issue to tackle. I was thinking to borrow some ideas from llvm > like AssertingVH to track use-after-delete. Again I need to measure the > penalties. And codegen cleanup is a monster in this respect. I saw some > light in the tunnel with the recent changes (haven't updated clang since a > while). > > > 4) BumpPtrAllocator is used everywhere, sometimes without calling the > no-op free, so even with a real allocator there would still be leaks > (meaning non-reused memory, in destruction all is freed to the system) > unless every allocation is matched with a free. > > This should be fixable, but I agree not easy to debug. AFAIK for AST nodes > it happens only in the ASTContext. > Vassil > > > > Yaron > > > > > 2014-08-04 12:26 GMT+03:00 Vassil Vassilev < > [email protected]>: > >> Hi Yaron, >> >> On 08/04/2014 10:31 AM, Yaron Keren wrote: >> >> Hi Vassil, >> >> If you decide to keep the last code posted as a cling patch, it could >> do with 'I' only instead of 'I' and 'current', and when MI is the first >> node the code should set MIChainHead but not set its Next. >> >> Thanks for pointing out, will do. >> >> >> To the point, ReleaseMacroInfo just releases the SmallVector Tokens >> memory if it wasn't small. >> It did not modify anything else. You could still removeMacro without >> ReleaseMacroInfo. >> >> Thanks for explaining. My code looks like this: >> >> void Preprocessor::removeMacro(IdentifierInfo *II, const MacroDirective >> *MD) { >> assert(II && MD); >> assert(!MD->getPrevious() && "Already attached to a MacroDirective >> history."); >> >> //Release the MacroInfo allocated space so it can be reused. >> MacroInfo* MI = MD->getMacroInfo(); >> if (MI) { >> ReleaseMacroInfo(MI); >> } >> Macros.erase(II); >> } >> >> IIUC I need to check if the small vector isSmall and if not then do a >> ReleaseMacro, or even this is redundant? >> >> >> There's lots of places in clang where memory is allocated and not >> released until destruction for performance. >> The whole AST for starters... >> >> >> It would be nice to early release the Tokens but In this context it >> would hardly move the needle. >> >> I agree. So I need to somehow implement it. >> >> >> cling memory use should going up every iteration due to this startegy, >> no? >> >> Yes, it grows. The context I want things removed is support of 'code >> unloading'. Say: >> [cling] #include "MyFile.h" >> [cling] MyClass m; m.do(); >> // Figure out that do is not what I want. I edit the file and do: >> [cling] #include "MyFile.h" // It would undo everything up to #include >> "MyFile.h" (inclusively). I want the memory to be reduced also. This is why >> I need to delete the macros and not only undef them. (The same holds for >> the AST) >> [cling] MyClass m; m.do(); // Here do and MyClass may have completely >> different implementation. >> >> Vassil >> >> >> Yaron >> >> >> >> >> >> 2014-08-04 10:47 GMT+03:00 Vassil Vassilev < >> [email protected]>: >> >>> Hi Richard, >>> Thanks for the fix! >>> >>> Unfortunately it doesn't help for cling case. I implement a >>> removeMacro routine using ReleaseMacroInfo. ReleaseMacroInfo allowed me to >>> implement efficiently the removal of a macro instead of dragging a long def >>> undef chains, for example. >>> IIUC it allowed some memory reduction in some cases for clang, too. Is >>> there any chance to keep the ReleaseMacroInfo upstream? >>> Vassil >>> >>> On 08/04/2014 01:50 AM, Richard Smith wrote: >>> >>> Fixed in a much more simple way in r214675. Thanks for reporting! >>> >>> >>> On Sun, Aug 3, 2014 at 11:52 AM, Vassil Vassilev <[email protected]> >>> wrote: >>> >>>> I will try just one more time and then shut up :) >>>> >>>> >>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp >>>> index 5f38387..000ea7a 100644 >>>> --- a/lib/Lex/PPDirectives.cpp >>>> +++ b/lib/Lex/PPDirectives.cpp >>>> @@ -94,6 +94,19 @@ >>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc, >>>> /// error in the macro definition. >>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) { >>>> // Don't try to reuse the storage; this only happens on error paths. >>>> + >>>> + // If this is on the macro info chain, avoid double deletion on >>>> teardown. >>>> + MacroInfoChain *current = MIChainHead; >>>> + while (MacroInfoChain *I = current) { >>>> + if (&(I->MI) == MI) { >>>> + I->Next = (I->Next) ? I->Next->Next : 0; >>>> + if (I == MIChainHead) >>>> + MIChainHead = I->Next; >>>> >>>> + break; >>>> + } >>>> + current = I->Next; >>>> + } >>>> + >>>> MI->~MacroInfo(); >>>> } >>>> >>>> >>>> On 03/08/14 20:47, Vassil Vassilev wrote: >>>> >>>> Hi, >>>> Sorry overlooked, thanks for pointing it out! >>>> I hope this is what we want. >>>> Vassil >>>> >>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp >>>> index 5f38387..000ea7a 100644 >>>> --- a/lib/Lex/PPDirectives.cpp >>>> +++ b/lib/Lex/PPDirectives.cpp >>>> @@ -94,6 +94,19 @@ >>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc, >>>> /// error in the macro definition. >>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) { >>>> // Don't try to reuse the storage; this only happens on error paths. >>>> + >>>> + // If this is on the macro info chain, avoid double deletion on >>>> teardown. >>>> + MacroInfoChain *current = MIChainHead; >>>> + while (MacroInfoChain *I = current) { >>>> + if (&(I->MI) == MI) { >>>> + I->Next = (I->Next) ? I->Next->Next : 0; >>>> + if (I == MIChainHead) >>>> + MIChainHead = I; >>>> + break; >>>> + } >>>> + current = I->Next; >>>> + } >>>> + >>>> MI->~MacroInfo(); >>>> } >>>> >>>> On 03/08/14 20:28, Yaron Keren wrote: >>>> >>>> Hi, >>>> >>>> MIChainHead is a pointer to the head of a linked list >>>> of MacroInfoChain nodes, each containing a MacroInfo and MacroInfoChain*. >>>> >>>> Why does the while loop modify MIChainHead on every iteration? >>>> MIChainHead should be modified only if it points to the node containing >>>> the removed MacroInfo MI. In all other cases it should not change. >>>> >>>> As it is now, the loop will always terminate with MIChainHead == >>>> nullptr. >>>> >>>> Yaron >>>> >>>> >>>> >>>> 2014-08-03 21:10 GMT+03:00 Vassil Vassilev <[email protected]>: >>>> >>>>> Hi Yaron, >>>>> Yes I meant double destruction. >>>>> Vassil >>>>> >>>>> On 03/08/14 20:08, Yaron Keren wrote: >>>>> >>>>> Hi Vassil, >>>>> >>>>> Do you mean double destruction (not deletion) of MacroInfo first >>>>> time in ReleaseMacroInfo and the second time in ~Preprocessor via >>>>> ~MacroInfoChain? >>>>> >>>>> while (MacroInfoChain *I = MIChainHead) { >>>>> MIChainHead = I->Next; >>>>> I->~MacroInfoChain(); >>>>> } >>>>> >>>>> or something else? >>>>> >>>>> Yaron >>>>> >>>>> >>>>> >>>>> 2014-08-02 23:05 GMT+03:00 Vassil Vassilev <[email protected]>: >>>>> >>>>>> Hi, >>>>>> In cases where ReleaseMacroInfo gets called and it doesn't cleanup >>>>>> the Preprocessor's MIChainHead can lead to double deletion. I am sending >>>>>> the patch that fixes the problem for me. >>>>>> Vassil >>>>>> >>>>>> >>>>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp >>>>>> index 5f38387..1a9b5eb 100644 >>>>>> --- a/lib/Lex/PPDirectives.cpp >>>>>> +++ b/lib/Lex/PPDirectives.cpp >>>>>> @@ -94,6 +94,14 @@ >>>>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc, >>>>>> /// error in the macro definition. >>>>>> void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) { >>>>>> // Don't try to reuse the storage; this only happens on error >>>>>> paths. >>>>>> + >>>>>> + // If this is on the macro info chain, avoid double deletion on >>>>>> teardown. >>>>>> + while (MacroInfoChain *I = MIChainHead) { >>>>>> + if (&(I->MI) == MI) >>>>>> + I->Next = (I->Next) ? I->Next->Next : 0; >>>>>> + MIChainHead = I->Next; >>>>>> + } >>>>>> + >>>>>> MI->~MacroInfo(); >>>>>> } >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>> >>>>> >>>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing >>>> [email protected]http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>> >>> >> >> > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
