<div dir="rtl"><div dir="ltr">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:</div>
<div dir="ltr"><div dir="ltr"><br></div><div dir="ltr"><div dir="ltr">NamedDecl **NamedChain = new (SemaRef.Context)NamedDecl*[Chaining.size()];</div><div dir="ltr">TemplateArgument *ArgumentPack = new (S.Context) TemplateArgument[Pack.New.size()]; <br>
</div><div dir="ltr"><div><br></div></div></div></div><div dir="ltr">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.</div>
<div dir="ltr"><div><br></div><div>Not all allocs are paired with a free. That could be found using the regular tools, if taught to watch for it.</div><div><br></div><div>Revising clang memory management is quite a task, while keeping top performance.</div>
<div>It does have the potential to lower clang peak memory usage while compiling big programs.</div><div><br></div><div>There is an excellent discussion of small object allocation in Chapter 4 of "Modern C++ Design: Generic Programming and Design Patterns Applied" / Andrei Alexandrescu.</div>
<div><br></div><div>Yaron<br></div><div><br></div><div><br></div><div dir="ltr"><br></div><div><br></div></div><div dir="ltr"><br></div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
<div dir="ltr">2014-08-04 13:42 GMT+03:00 Vassil Vassilev <span dir="ltr"><<a href="mailto:vasil.georgiev.vasilev@cern.ch" target="_blank">vasil.georgiev.vasilev@cern.ch</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 .8ex;border-left:1px #ccc solid;border-right:1px #ccc solid;padding-left:1ex;padding-right:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Hi Yaron,<br>
Thanks for raising those very interesting points!<div class=""><br>
On 08/04/2014 12:12 PM, Yaron Keren wrote:<br>
</div></div><div class="">
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr">Hi,</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">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.<br>
</div>
</div>
</blockquote></div>
Understood.<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr">
</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">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.</div>
</div>
</blockquote></div>
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?<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">I have looked into this issue for Ceemple which
has similar need as cling for memory reclaim and gave it up
for now. </div>
<div dir="ltr">It's actually quite hard to make clang reclaim
memory before destruction, since</div>
</div>
</blockquote></div>
I couldn't agree more.<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">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.</div>
</div>
</blockquote></div>
I think using a slab alloc may be not that bad for us.<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">2) Freeing the memory slows down performance even
more. BumpPtrAllocator free is a no-op.</div>
</div>
</blockquote></div>
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.<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">3) Actually releasing the memory may cause
use-after-free bugs which are not seen now since the AST
memory is never really released.</div>
</div>
</blockquote></div>
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).<div class=""><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">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.</div>
</div>
</blockquote></div>
This should be fixable, but I agree not easy to debug. AFAIK for AST
nodes it happens only in the ASTContext.<span class="HOEnZb"><font color="#888888"><br>
Vassil</font></span><div><div class="h5"><br>
<br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">Yaron</div>
<div dir="ltr"><br>
</div>
<div dir="ltr"><br>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">2014-08-04 12:26 GMT+03:00 Vassil Vassilev <span dir="ltr"><<a href="mailto:vasil.georgiev.vasilev@cern.ch" target="_blank">vasil.georgiev.vasilev@cern.ch</a>></span>:</div>
<blockquote class="gmail_quote" style="margin:0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;border-right-width:1px;border-right-color:rgb(204,204,204);border-right-style:solid;padding-left:1ex;padding-right:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Hi Yaron,
<div><br>
On 08/04/2014 10:31 AM, Yaron Keren wrote:<br>
</div>
</div>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr">Hi Vassil,</div>
<div dir="ltr"><br>
</div>
<div>
<div dir="ltr">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.</div>
</div>
</div>
</blockquote>
Thanks for pointing out, will do.
<div><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><br>
</div>
<div dir="ltr">To the point, <span style="font-family:arial,sans-serif;font-size:12.727272033691406px">ReleaseMacroInfo
just releases the SmallVector Tokens memory if
it wasn't small.</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">It
did not modify anything else</span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">.
You could still </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">removeMacro</span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"> without </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">ReleaseMacroInfo.</span></div>
</div>
</blockquote>
</div>
Thanks for explaining. My code looks like this:<br>
<br>
void Preprocessor::removeMacro(IdentifierInfo *II, const
MacroDirective *MD) {<br>
assert(II && MD);<br>
assert(!MD->getPrevious() && "Already
attached to a MacroDirective history.");<br>
<br>
//Release the MacroInfo allocated space so it can be
reused.<br>
MacroInfo* MI = MD->getMacroInfo();<br>
if (MI) {<br>
ReleaseMacroInfo(MI);<br>
}<br>
Macros.erase(II);<br>
}<br>
<br>
IIUC I need to check if the small vector isSmall and if
not then do a ReleaseMacro, or even this is redundant?
<div><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">There's
lots of places in clang where memory is
allocated and not released until destruction
for performance. </span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">The
whole AST for starters... <br>
</span></div>
</div>
</blockquote>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">It
would be nice to early release the Tokens but </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">In
this context it would hardly move the needle.</span></div>
</div>
</blockquote>
</div>
I agree. So I need to somehow implement it.
<div><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">cling
memory use should going up every iteration due
to this startegy, no?</span></div>
</div>
</blockquote>
</div>
Yes, it grows. The context I want things removed is
support of 'code unloading'. Say:<br>
[cling] #include "MyFile.h"<br>
[cling] MyClass m; m.do();<br>
// Figure out that do is not what I want. I edit the
file and do:<br>
[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)<br>
[cling] MyClass m; m.do(); // Here do and MyClass may
have completely different implementation.<span><font color="#888888"><br>
<br>
Vassil</font></span>
<div>
<div><br>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">Yar</span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">on</span></div>
<div dir="ltr"><br>
</div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">2014-08-04 10:47 GMT+03:00
Vassil Vassilev <span dir="ltr"><<a href="mailto:vasil.georgiev.vasilev@cern.ch" target="_blank">vasil.georgiev.vasilev@cern.ch</a>></span>:</div>
<blockquote class="gmail_quote" style="margin:0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;border-right-width:1px;border-right-color:rgb(204,204,204);border-right-style:solid;padding-left:1ex;padding-right:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Hi Richard,<br>
Thanks for the fix!<br>
<br>
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.<br>
IIUC it allowed some memory reduction
in some cases for clang, too. Is there
any chance to keep the ReleaseMacroInfo
upstream? <br>
<span><font color="#888888"> Vassil</font></span>
<div>
<div><br>
On 08/04/2014 01:50 AM, Richard
Smith wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="ltr">Fixed in a much more
simple way in r<span style="color:rgb(0,0,0)">214675.
Thanks for reporting!</span></div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Sun,
Aug 3, 2014 at 11:52 AM, Vassil
Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>I will try just one
more time and then shut up
:)
<div><br>
<br>
diff --git
a/lib/Lex/PPDirectives.cpp
b/lib/Lex/PPDirectives.cpp<br>
index 5f38387..000ea7a
100644<br>
---
a/lib/Lex/PPDirectives.cpp<br>
+++
b/lib/Lex/PPDirectives.cpp<br>
@@ -94,6 +94,19 @@
Preprocessor::AllocateVisibilityMacroDirective(SourceLocation
Loc,<br>
/// error in the macro
definition.<br>
void
Preprocessor::ReleaseMacroInfo(MacroInfo
*MI) {<br>
// Don't try to reuse
the storage; this only
happens on error paths.<br>
+<br>
+ // If this is on the
macro info chain, avoid
double deletion on
teardown.<br>
+ MacroInfoChain
*current = MIChainHead;<br>
+ while (MacroInfoChain
*I = current) {<br>
+ if (&(I->MI)
== MI) {<br>
+ I->Next =
(I->Next) ?
I->Next->Next : 0;<br>
+ if (I ==
MIChainHead)<br>
</div>
+ MIChainHead =
I->Next;
<div><br>
+ break;<br>
+ }<br>
+ current =
I->Next;<br>
+ }<br>
+<br>
MI->~MacroInfo();<br>
}<br>
<br>
<br>
</div>
<div>
<div> On 03/08/14 20:47,
Vassil Vassilev wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div>Hi,<br>
Sorry overlooked,
thanks for pointing
it out!<br>
I hope this is
what we want.<br>
Vassil<br>
<br>
diff --git
a/lib/Lex/PPDirectives.cpp
b/lib/Lex/PPDirectives.cpp<br>
index
5f38387..000ea7a
100644<br>
---
a/lib/Lex/PPDirectives.cpp<br>
+++
b/lib/Lex/PPDirectives.cpp<br>
@@ -94,6 +94,19 @@
Preprocessor::AllocateVisibilityMacroDirective(SourceLocation
Loc,<br>
/// error in the
macro definition.<br>
void
Preprocessor::ReleaseMacroInfo(MacroInfo
*MI) {<br>
// Don't try to
reuse the storage;
this only happens on
error paths.<br>
+<br>
+ // If this is on
the macro info
chain, avoid double
deletion on
teardown.<br>
+ MacroInfoChain
*current =
MIChainHead;<br>
+ while
(MacroInfoChain *I =
current) {<br>
+ if
(&(I->MI) ==
MI) {<br>
+ I->Next =
(I->Next) ?
I->Next->Next
: 0;<br>
+ if (I ==
MIChainHead)<br>
+ MIChainHead
= I;<br>
+ break;<br>
+ }<br>
+ current =
I->Next;<br>
+ }<br>
+<br>
MI->~MacroInfo();<br>
}<br>
<br>
On 03/08/14 20:28,
Yaron Keren wrote:<br>
</div>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr">Hi,</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">MIChainHead
is a pointer to
the head of a
linked list
of MacroInfoChain
nodes, each
containing
a MacroInfo
and MacroInfoChain*.<br>
</div>
<div dir="ltr"> <br>
</div>
<div dir="ltr">Why
does the while
loop
modify MIChainHead
on every
iteration?</div>
<div dir="ltr">MIChainHead
should be
modified only if
it points to the
node containing
the removed <span style="font-size:12.727272033691406px;font-family:arial,sans-serif">MacroInfo
MI. </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">In
all other
cases it
should not
change.</span></div>
<div dir="ltr"><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div>
<div dir="ltr">As
it is now, the
loop will always
terminate
with MIChainHead
== nullptr.<br>
</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">Yaron</div>
<div dir="ltr"><br>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">2014-08-03
21:10
GMT+03:00
Vassil
Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span>:</div>
<blockquote class="gmail_quote" style="margin:0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;border-right-width:1px;border-right-color:rgb(204,204,204);border-right-style:solid;padding-left:1ex;padding-right:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div>Hi Yaron,<br>
Yes I meant
double
destruction.<span><font color="#888888"><br>
Vassil</font></span>
<div>
<div><br>
On 03/08/14
20:08, Yaron
Keren wrote:<br>
</div>
</div>
</div>
<div>
<div>
<blockquote type="cite">
<div dir="rtl">
<div dir="ltr">Hi
Vassil,</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">Do
you mean
double
destruction
(not deletion)
of <span style="font-family:arial,sans-serif;font-size:12.727272033691406px">MacroInfo
first time in </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">ReleaseMacroInfo </span><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">and
the second
time in </span>~Preprocessor
via
~MacroInfoChain?</div>
<div dir="ltr"><br>
</div>
<div dir="ltr">
<div dir="ltr">
while
(MacroInfoChain
*I =
MIChainHead) {</div>
<div dir="ltr">
MIChainHead
= I->Next;</div>
<div dir="ltr">
I->~MacroInfoChain();</div>
<div dir="ltr">
}</div>
<div><br>
</div>
<div>or
something
else?</div>
<div><br>
</div>
<div>Yaron<br>
</div>
</div>
<div dir="ltr"><br>
</div>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">2014-08-02
23:05
GMT+03:00
Vassil
Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span>:</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi,<br>
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.<br>
Vassil<br>
<br>
<br>
diff --git
a/lib/Lex/PPDirectives.cpp
b/lib/Lex/PPDirectives.cpp<br>
index
5f38387..1a9b5eb
100644<br>
---
a/lib/Lex/PPDirectives.cpp<br>
+++
b/lib/Lex/PPDirectives.cpp<br>
@@ -94,6
+94,14 @@
Preprocessor::AllocateVisibilityMacroDirective(SourceLocation
Loc,<br>
/// error in
the macro
definition.<br>
void
Preprocessor::ReleaseMacroInfo(MacroInfo
*MI) {<br>
// Don't
try to reuse
the storage;
this only
happens on
error paths.<br>
+<br>
+ // If this
is on the
macro info
chain, avoid
double
deletion on
teardown.<br>
+ while
(MacroInfoChain
*I =
MIChainHead) {<br>
+ if
(&(I->MI)
== MI)<br>
+
I->Next =
(I->Next) ?
I->Next->Next
: 0;<br>
+
MIChainHead =
I->Next;<br>
+ }<br>
+<br>
MI->~MacroInfo();<br>
}<br>
<br>
_______________________________________________<br>
cfe-commits
mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
cfe-commits mailing list
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a>
</pre>
</blockquote>
<br>
</div>
</div>
</div>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</div></div></div>
</blockquote></div><br></div>