<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Yaron,<br>
Thanks for raising those very interesting points!<br>
On 08/04/2014 12:12 PM, Yaron Keren wrote:<br>
</div>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<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>
Understood.<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
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?<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
I couldn't agree more.<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
I think using a slab alloc may be not that bad for us.<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
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.<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
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).<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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>
This should be fixable, but I agree not easy to debug. AFAIK for AST
nodes it happens only in the ASTContext.<br>
Vassil<br>
<br>
<blockquote
cite="mid:CANa4zJohPLO5jFWXUtJQZUhhFGQW3uX5UmpLbFd9CdoR_kjPRg@mail.gmail.com"
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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true" 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
moz-do-not-send="true" 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
moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a
moz-do-not-send="true"
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 moz-do-not-send="true" href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>
<a moz-do-not-send="true" 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 moz-do-not-send="true"
href="mailto:cfe-commits@cs.uiuc.edu"
target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
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>
</body>
</html>