<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 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 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 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 dir="ltr"><br></div><div dir="ltr">2) Freeing the memory slows down performance even more. BumpPtrAllocator free is a no-op.</div><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 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 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>