On Thu, 11 Nov 2004, Morten Ofstad wrote:> Well, I already tried that, but the destructors crash because they are > referencing other things which are being destroyed - Constants are Users > of each other and there is no easy way to destroy them in the right > order.There are ways around this, but it turns into a two-pass operation: loop over all constants to drop their uses, then loop over them to delete them all.> The same problem happens with the derived types because the > reference counting is checking if the contained types are abstract when > deleting them... No, what I was proposing with the mempool is (at exit) > to free the memory all the constants and types live in without calling > any destructors whatsoever. The overhead would be minimal, since we're > just providing a special new operator that allocates the singletons in a > known place, so no tracking has to be done per object.Ok, I'm not following then. You want to change the per-class operator new in the type and constant classes to avoid the VC tracking operator new? I'd like to find a way to do this that does not penalize non-VC users at all.> It would also solve another problem -- We generate new shader code when > the user changes parameters, because the shader will be executed > millions of times it makes sense to recompile it with changed constants > to get maximum optimization. But if some of these parameters are floats, > and there is no way to destroy constants in LLVM we have a serious > memory leak situation here. Having the ability to flush all constants > and types and clear the TypeMaps/ValueMaps would be very useful to us.If this is a serious issue, it would be straight-forward to add a global function to flush out unused constants. I would be happy to accept a patch to do that.> Unless you have another suggestion for fixing this scenario I will go > ahead and implement this solution - even if you don't want to take the > patches for it, it's something I simply can't live without.If you can come up with a solution for the first one (to satisfy your 'memory leak detector') that does not penalize users of other compilers, I would be happy to take the patch for it too.> Just to end on a positive note: It was very easy to generate the LLVM > intermediate rep. from our code - I have simple code generation working > already, with bindings to global data in our program and so on. If only > I could sort out these memory leak problems everything would be very > very sunny...Great!!! -Chris -- http://llvm.org/ http://nondot.org/sabre/
Chris Lattner wrote:>>It would also solve another problem -- We generate new shader code when >>the user changes parameters, because the shader will be executed >>millions of times it makes sense to recompile it with changed constants >>to get maximum optimization. But if some of these parameters are floats, >>and there is no way to destroy constants in LLVM we have a serious >>memory leak situation here. Having the ability to flush all constants >>and types and clear the TypeMaps/ValueMaps would be very useful to us. > > If this is a serious issue, it would be straight-forward to add a global > function to flush out unused constants. I would be happy to accept a > patch to do that.Here is an implementation of this -- there are some things you might not like about it: 1) I had to make the private destroyConstantImpl method public to access it from the template class ValueMap since there is no good way to declare it as a friend. 2) I keep the types in a std::vector so I can easily drop all references and delete them without going through all the type maps - this adds a little bit of overhead, but there shouldn't be that many types (compared to constants), so to me it seems acceptable. This also means I can get by with a 'friend' declaration to get at the private data of the types when I want to forcefully drop references. I dropped the memory pool thing because the use lists always 'new' one element, so I could not avoid calling the destructors of the constants even if they have no (real) uses... m. -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: memcleanup.txt URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20041115/1edddba3/attachment.txt>
On Mon, 15 Nov 2004, Morten Ofstad wrote:> > If this is a serious issue, it would be straight-forward to add a global > > function to flush out unused constants. I would be happy to accept a > > patch to do that. > > Here is an implementation of this -- there are some things you might not > like about it: > > 1) I had to make the private destroyConstantImpl method public to access > it from the template class ValueMap since there is no good way to > declare it as a friend.That's fine in principle, though if you make the clearAllValueMaps function be a static method in Type this is unneeded I think.> 2) I keep the types in a std::vector so I can easily drop all references > and delete them without going through all the type maps - this adds a > little bit of overhead, but there shouldn't be that many types (compared > to constants), so to me it seems acceptable. This also means I can get > by with a 'friend' declaration to get at the private data of the types > when I want to forcefully drop references.Yeah, that's kinda gross. :(> I dropped the memory pool thing because the use lists always 'new' one > element, so I could not avoid calling the destructors of the constants > even if they have no (real) uses...Ok, here's some feedback. This patch looks like a combination of several different things. If you resubmit the patch in pieces, we can get the undebatable ones applied first: 1. Some changes of unsigned to size_t and other things to make VC happy. These are fine. 2. The clearAllValueMaps related changes. These are also fine, but please make clearAllValueMaps be a static function in the Constant class, Constant::ClearAllValueMaps(). 3. The LeakDetector changes: These are not ok, because the order of construction of the static objects is not defined (these maps may be initialized after other things that put stuff into them). If you change the static objects in getObjects() and getLLVMObjects() to be LLVMObjects themselves instead of pointers, I think everything should work fine (they will be initialized on first use that way). In other words, getObjects becomes: Objects &getObjects() { static Objects Objs; return Objs; } 4. The Types change, including the vector. As you guessed, I'm not a fan of this at all: it adds overhead to the normal case. Don't the *Types maps contain all of the information that you need to do the clearing? In clearAllTypeMaps (which should become a static method in Type, allowing you to avoid the friend issues), as a first pass, can't you loop over FunctionTypes, PointerTypes, etc and build the vector there? As a general note, you don't need to use '(void)' as arguments in C++, this is a C thing. Please just use '()'. -Chris -- http://llvm.org/ http://nondot.org/sabre/