[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)
Yaron Keren
yaron.keren at gmail.com
Mon Aug 4 04:35:29 PDT 2014
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 <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>:
>
>> 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>:
>>
>>> 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>
>>> 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>:
>>>>
>>>>> 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>:
>>>>>
>>>>>> 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
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing listcfe-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> 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/63d89050/attachment.html>
More information about the cfe-commits
mailing list