[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)
Vassil Vassilev
vvasilev at cern.ch
Mon Aug 4 13:46:35 PDT 2014
On 04/08/14 13:35, Yaron Keren wrote:
> 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()];
I see. For me the big unknown is how to measure memory footprints
improvement/degradation when playing with the allocators. Is there
anything that can be used as a golden reference?
>
> 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.
In my case, I iterate over the entities and I can call the dealloc.
Maybe it can be generalized for mainline also.
>
> 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.
Would clang community be interested in work in this direction?
>
> There is an excellent discussion of small object allocation in Chapter
> 4 of "Modern C++ Design: Generic Programming and Design Patterns
> Applied" / Andrei Alexandrescu.
Thanks for the pointer. Would this be the preferred approach in
implementing the memory management?
Vassil
>
> Yaron
>
>
>
>
>
>
>
>
> 2014-08-04 13:42 GMT+03:00 Vassil Vassilev
> <vasil.georgiev.vasilev at cern.ch <mailto:vasil.georgiev.vasilev at cern.ch>>:
>
> 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
>> <vasil.georgiev.vasilev at cern.ch
>> <mailto:vasil.georgiev.vasilev at cern.ch>>:
>>
>> 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
>>> <vasil.georgiev.vasilev at cern.ch
>>> <mailto:vasil.georgiev.vasilev at cern.ch>>:
>>>
>>> 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
>>>> <vvasilev at cern.ch <mailto:vvasilev at cern.ch>> 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
>>>>>> <vvasilev at cern.ch <mailto:vvasilev at cern.ch>>:
>>>>>>
>>>>>> 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
>>>>>>> <vvasilev at cern.ch <mailto:vvasilev at cern.ch>>:
>>>>>>>
>>>>>>> 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
>>>>>>> cfe-commits at cs.uiuc.edu
>>>>>>> <mailto:cfe-commits at cs.uiuc.edu>
>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> <mailto:cfe-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/45033429/attachment.html>
More information about the cfe-commits
mailing list