Yaron
2014-08-04 13:42 GMT+03:00 Vassil Vassilev
<[email protected] <mailto:[email protected]>>:
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
