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
<[email protected] <mailto:[email protected]>>:
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
<[email protected]
<mailto:[email protected]>>:
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
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>>:
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
<[email protected] <mailto:[email protected]>>:
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
[email protected]
<mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
