memory leak with xlCoerce

Developer
Jul 29, 2011 at 7:19 PM

I've just gotten started using your library and it has definitely helped making an Excel xll much easier!  However, I've noticed it seems to leak memory when using the xlCoerce function through your ExcelX helper function in user defined functions.  For example:

EXAMPLE USING C API DIRECTLY (NO MEMORY LEAK!):

    //assume Excel gave you 'LPXLOPERX x'
    XLOPER12 xMulti;
    XLOPER12 xType;
    xType.xltype = xltypeInt;
    xType.val.w = xltypeMulti;
    if (xlretSuccess != Excel12(xlCoerce, &xMulti, 2, x, &xType)) {
        return 0;
    }
    // do stuff...
    Excel12(xlFree, 0, 1, &xMulti);

EXAMPLE USING NXLL WRAPPER (MEMORY LEAK!):

    //assume Excel gave you 'LPXLOPERX x'
    OPERX xMulti;
    try {
        xMulti= ExcelX(xlCoerce, *x, IntX(xltypeMulti));
    } catch (...) {
        return 0;
    }
    //do stuff...
    ExcelX(xlFree, xMulti);

 

I haven't had time to read through your code enough to figure out why this memory leak is occurring, but I assume the OPERX returned by the ExcelX() call is actually a copy of the memory allocated by Excel and thus the original memory is not actually being freed by the ExcelX(xlFree,...) call (or the OPERX destructor).  Is there a better approach to go about using your library with the xlCoerce call, or should I just stick to directly using the C api when making such calls?

Coordinator
Jul 29, 2011 at 7:47 PM

Your intuition is correct. Do not call ExcelX(xlFree, xMulti) because the OPERX destructor calls that for you. With nxll you never have to worry about Excel's protocol for managing memory. Just declare an object, use it, and let C++ do the cleanup when it goes out of scope.

If you look behind the scenes, you will find the LOPER class that handles OPER l-values. It is a smart pointer class like std::shared_ptr. You should never need to use that directly.

One of my design goals was to make OPERs work like built-in C++ data types. You should never need to call new/delete or xlFree.

Pro Tip: You don't need IntX(xltypeMulti) because Excel always converts ints to doubles. Use NumX, or even better, OPERX to create temporary objects to pass to ExcelX.

Developer
Jul 29, 2011 at 9:52 PM
Edited Jul 29, 2011 at 9:52 PM

Hi Keith, thanks for the quick reply and tips!  Your library has been very useful.  However, maybe I wasn't being entirely clear.  The memory leak doesn't seem to be related to the object returned from ExcelX.  For example, the following user defined function will leak memory.  You can verify by using this function in Excel and using a very large input x and repeatedly recalculating to see the Excel memory use soar through the roof on each recalculation until it crashes.  If you manually call Excel12(xlCoerce...) and Excel12(xlFree...) with a raw XLOPER12 it does not have this memory leak problem:

 

static AddInX add_returnNum(
    FunctionX(XLL_DOUBLEX, _T("?returnNum"), _T("ReturnNum"))
    .Arg(XLL_LPXLOPERX, _T("Irrelevant"), _T("This is irrelevant."))
);

double WINAPI
returnNum(LPXLOPERX x)
{
#pragma XLLEXPORT
    //some pointless code to cause a memory leak
    OPERX xMulti;
    try { 
        xMulti = ExcelX(xlCoerce, *x, OPERX(xltypeMulti));
    } catch (std::exception ) {
        return 0;
    }
   
    return xll::to_number<XLOPERX>(xMulti);
}

 

The memory from xMulti seems to be released properly; however, some memory somewhere else isn't being released properly.  I can't find an obvious reason for this, but I think it may be something related to the fact that Excel is allocating the memory for us when we call xlCoerce and thus we need to call xlFree to tell Excel to release it properly... however calling ExcelX(xlFree, xMulti) potentially doesn't work because we have made a copy of the XLOPER12 returned by Excel when the ExcelX function returns...

thanks for the help!

Coordinator
Jul 29, 2011 at 10:33 PM

I'm not able to reproduce this using the latest nxll library. I've run your code in Excel 2003, 2007, and 2010. I do see a slight creep in memory usage in Excel 2007. My input is a range with 1000 calls to rand(). BTW, you might find Baby Huey handy for banging on spreadsheets. You can find him at http://xllutility.codeplex.com. There is a memory leak in RAND.MULTI and RAND.STR, so don't use them for testing.

Try replicating with the latest source from subversion and send me the spreadsheet you are looking at. My e-mail is kal@kalx.net.

Another Pro Tip: You can just say 'return xMulti;' No need to call xll::to_number. The C++ compiler will call OPER::operator double() for you which will then call to_number.

The idea behind that is to make 'if (xMulti) { ... }' work right. If anything in the xltypeMulti evaluates to 0 or false, it will evaluate to false. Right now I just multiply all the numbers together, but based on your example I've decided to change that to something more intuitive. As soon as my intuition tells me what that is. I'm open to suggestions.

Your feedback is appreciated.

Coordinator
Jul 30, 2011 at 2:58 AM

I've updated http://kalx.net/dnload/setup.zip with the latest code if that is easier for you.

Developer
Jul 30, 2011 at 6:20 AM
Edited Jul 30, 2011 at 6:58 AM

Hi again, thanks for the setup update and tip!  I used the setup installer earlier to make life easier, but I did pull a fresh copy from the svn before looking back on here :P.  I uploaded a complete test project here illustrating the issue:

http://wikisend.com/download/227580/nxll_test.zip

I'm not sure 1000 lines is enough to spike a noticeable memory jump... so I built a massive spreadsheet to make it a little more obvious (at least my copy of Excel 2010 ran out of memory and crashed pretty quickly).  The test spreadsheet and precompiled xll are under the testdocs folder.  Alternatively, I also included my entire MSVC project, that will hopefully build without tweaking anything...  I just used the nxll template project 'XllProject1' and added two UDFs to the 'functions.cpp' file--XLL.LEAK and XLL.NOLEAK.  They consist of an implementation of the function from my previous post with XLL.LEAK using the OPERX wrappers and XLL.NOLEAK manually using the C API's XLOPER12 structs.  The behavior on my machine is that every time the XLL.LEAK functions are recomputed by Excel additional memory is leaked, while every time the XLL.NOLEAK functions are called no memory appears to be leaked.

Hopefully I'm just missing something obvious.  Thanks for taking a look!

Developer
Jul 30, 2011 at 6:29 AM

And as far as the xll::to_number for xltypeMulti... that is an interesting question as to what the "expected behavior" would be since there's no one correct interpretation of that.  I do agree that the if(xMulti) {...} use to test for 'false' values is nice.  Multiplying is an easy way to do this, but I know it threw me off a little bit because it returns overflow errors really easily from all the consecutive multiplies.  Maybe just go with the Excel approach to single cell array functions and just return whatever happens to be in the (0,0) position?  Although this kills the current if(xMulti) behavior...


I know for me personally I would like an easy way to test if the xMulti contained all valid numbers.  Maybe to_number(xMulti) could indicate if all values were numbers, rather than its current behavior of indicating that all values are {numbers!=0 || bools==TRUE || strings!=""} which I don't see as clear a use case for... although it does mess with the if(xMulti) behavior.  Maybe there could be a separate to_bool/operator bool() from to_number/operator double() function, although I haven't looked around in your code enough nor know C++ well enough to know the complications of that...


I dunno, I'll think about it... challenging to think about the generic use case.  Kudos to you for tackling this project!

Coordinator
Jul 30, 2011 at 11:34 AM

I like your idea of adding operator bool() but it makes operator==() ambiguous. Try adding 'operator bool() const { return true; }' to the OPER class to see what happens.

You are correct about the underflow problem. I change it to return to_number(x->val.array[0]) if x is 1 x 1 per your suggestion, and either 1 or 0 otherwise based on 0 != to_number(x->val.array[i]) for all i. It now short circuits when it finds a 0. I guess that is slightly less mystifying behavior that still does the right thing in an 'if' condition.

Your example works fine and I am definitely seeing memory spikes. I also see them settle down if I just wait a bit. Let me reboot into a quiescent state and take a better look.

I've been developing this over the past 10 year for use with my clients. They live in Excel and I like tools that are nice to use so I really wrote it for myself. Making it easy for other people to use is the hard part.

Coordinator
Jul 30, 2011 at 1:14 PM

I'm not seeing any leaks on my side. By leak I mean memory being allocated and not freed. I do see malloc and free being called for 32MB x 10 on each recalc. My guess is that Windows 7 gobbles up all the RAM it can get its grubby paws on. When I exit Excel the memory drops back to its original level. Do Ctrl-F in the output window and search for 'leak'. See xll/debug.cpp for the technique I use to detect leaks. If there is a leak then the destructor for the crtDbg object should find it.

Just to be sure, I trundled out Baby Huey and had him bang on the spreadsheet 100,000 times. It definitely pegs the RAM, but it doesn't crash.

Remember how I told you you never need to use LOPER's? Well, here is where they come in handy. Declare xMulti as LOPERX and you will get the same memory profile as your XLL.NOLEAK function. The LOPER lets Excel manage the memory and calls back using xlFree when the last l-value goes out of scope.

Developer
Jul 30, 2011 at 9:43 PM
Edited Jul 30, 2011 at 9:50 PM

Ah, yes, I think that is a more intuitive behavior for to_num() on a xlMulti type!  And now that you mention it, I think I do recall seeing a little reading on the dangers of operator bool() and operator==().  I'm a relative newcomer to C++ so still trying to get a handle on all the proper idioms :)

And hrmm... I'm a little confused as to why that test behaved differently for you and I (Excel ran out of memory and crashed for me after banging on the spreadsheet about 10 times, nonetheless 100,000 times!).  However, I started diving through the library a little bit and I think I found the source of the leak... so maybe we somehow have different versions of the library, even though I'm pretty sure I checked out the latest copy from the svn.  Actually, your comment in the default constructor for LXOPER tipped me off:

    LXOPER()
    {
        xltype = xltypeNil;
        own(true); // so LXOPER x; Excel4(fun, &x, ...); works right
    }

I noticed that in the ExcelX functions the return value was an LXOPER instantiated from a raw XLOPER(12) which means that the LXOPER returned from ExcelX doesn't "own" its value, so the call to xlFree in the destructor of LXOPER never actually occurs.  So here is my tweak to (one of) the ExcelX functions which eliminates the memory leak problem for me:

    template<class X>
    LXOPER<X> Excel(int f, const X& x0, const X& x1)
    {
        //X x; //<-----original
        LXOPER<X> x; //<------new

        throw_if (traits<X>::Excel(f, &x, 2, &x0, &x1));

        return x;
    }

So let me know what you think, or if that tweak introduces any bugs that you can think of!  (Or if its just some sort of versioning issue :P)

For reference, going with your suggestion of using LOPER to avoid unnecessary copying of the array data, my final version of the code that leaked prior to the ExcelX tweak and didn't after is as follows:

 

double WINAPI
xll_noleakMemory2(LPXLOPERX x)
{
#pragma XLLEXPORT
    //some pointless code with no memory leak (with the tweaked ExcelX)!
    LOPERX xMulti;
    try {
        xMulti = ExcelX(xlCoerce, *x, OPERX(xltypeMulti));
    } catch (std::exception) {
        return 0;
    }

    return xMulti; //without the ExcelX tweak, xMulti.owner==false here and the memory allocated by xlCoerce is never freed by xMulti's destructor 
}

 

Thanks for the help!

Coordinator
Jul 31, 2011 at 8:13 AM

Excel (the function in excel.h) returns an LXOPER<X>. That should call the constructor when you return X x.

I think C++ is the eighth wonder of the world. It is damn hard to get it right, but it is astoundingly powerful when you do.

I used the zip file you provided to do the tests. Please do that again if you are still seeing leaks. If you are using subversion to pick up the latest code, that will make it easier for me to fix bugs.

Developer
Jul 31, 2011 at 7:01 PM
Edited Jul 31, 2011 at 7:02 PM

Hrmm... I confirmed that I have the latest code from http://nxll.codeplex.com/SourceControl/list/changesets, so I do think what I described above is a minor bug.  When the Excel function returns a LXOPER<X> constructed from an LOPERX, it does call one of LXOPER<X>'s constructors, but I don't think it does what we want.  This is what gets called:

LXOPER(const X& x)
{
	own(false);
	assign(x);
}

This constructor doesn't assign ownership of the allocated memory to the LOPERX returned by Excel.  Thus later on down the road when lfree() is called by LOPERX's destructor (or otherwise), xlFree is never called resulting in a memory leak.

    // l-value really gets freed
    void lfree(void)
    {
        if (owner) { //<---this is always false for LXOPER's returned by ExcelX(...)
            owned(-1);
            xll::traits<X>::Excel(xlFree, 0, 1, this);
        }
    }

Hopefully that was specific enough to help in the always fun adventure that is bug hunting :)

Coordinator
Aug 1, 2011 at 2:35 AM

Good catch colinrodriguez. Your analysis is completely correct. My comment is right :-), but the code for the ExcelX function is not. I've incorporated your proposed solution.

I've found other problematic use cases (ok, bugs) so give me a a few days to ponder before updating the source code. Your help is much appreciated.

Developer
Aug 1, 2011 at 7:33 PM
Edited Aug 1, 2011 at 7:37 PM

Haha, no problem.  I see you checked in a more robust/explicit fix this morning :).   Thanks again for the awesome library!  I've been testing out a couple different methods/libraries for creating an Excel add-on, and so far using the XLL approach paired with your library is my favorite because it gives you all the low level access without hiding everything behind magic button code generators (like all the commercial products I've tested out seem to) while keeping the C++ complexity in check with the nice wrapper utility classes.

If you don't mind, I have a quick question about your experience with best practices concerning memory management when returning values to Excel involving large amounts of dynamic memory.  Basically, the normal example I've seen when returning an array to Excel is to return a pointer to a static XLOPER(12) (or OPERX with your library) within the UDF, but this leaves a bunch of allocated memory hanging around until the next time the UDF is called.  So my first hack at actually clearing this memory out after the function call is complete is as follows:

#pragma comment(linker, "/export:xlAutoFree12@4=xlAutoFree12")
void WINAPI xlAutoFree12(LPXLOPER12 pxFree)
{
    OPERX *x = reinterpret_cast<OPERX*>(pxFree); //assume we only are using OPERX's
    x->xltype &= 0x0FFF; // mask away xlbitDLLFree
    delete x;
}

LPOPERX WINAPI
xll_testmem()
{
#pragma XLLEXPORT
    OPERX *xReturn = new OPERX();

    xReturn->resize(1000,1000);
    //... do some stuff to fill xReturn with a HUGE array !...

    xReturn->xltype |= xlbitDLLFree;
    return xReturn;
}

 

Does dynamically creating an OPERX and then using xllAutoFree to delete it fall within your experience of good practice?  Or do you use a better approach?  If you can't tell, I'm trying to build something which deals with some HUGE matrices, and I really need to make sure I'm not taking up any additional memory if not necessary :P

Thanks!

Coordinator
Aug 1, 2011 at 10:13 PM

Glad you like it. Would you mind if I use your first paragraph for a testimonial. Marketing types seem to think those are useful and the more people I get using this, the better.

Your undestanding about memory that you allocate is correct. Set the xlbitDLLFree flag before returning and Excel calls xlAutoOpen with your pointer after it's had its way with it.

If Excel allocates the memory, set the xlbitXLFree flag and Excel will do the clean up.

There are a few more tricks. You can use the xll::handle class and take out your own garbage. One idiom is '=GC(USE(CREATE(...)))' where CREATE is your function that allocates a bunch of memory, USE is your function to do something with that data, and GC calls your garbage collector on the handle<T> returned by USE. This gets Excel to call things for you instead of manually doing it yourself. The down side is the data only exists during the call to USE so it only works with ephemeral data. There are also tricks with DEPENDS from xllutility that might be handy to manually specify calculation order.

If you are using only numbers, you should be using the FP data type. It takes up 8 bytes per number instead of 24 bytes per OPER. It has an underlying array of doubles so it plays nicely with existing numerical librarys. See xllblas, xlllapack, and xllgsl for example uses.

I've checked in some code for doing AutoFree stuff, but it is not in good shape yet. If you are using subversion, just do an update.

P.S. Use x->xltype&(~xlbitDLLFree) and catch const std::exception&. You need const references in case someone subclasses std::exception, like I'm doing.

Developer
Aug 2, 2011 at 2:49 AM

Good stuff!  Thanks for the examples, I've gotten all the memory management working to my liking now :).  Of course, now I'll have to investigate this FP data type I overlooked, because it did seem like I was doing an awful lot of unnecessary work to get the values out of xlMulti and into a useful data structure to do some linear algebra on...

And, of course you may use the testimonial!  I'll keep in touch about other issues/clarifications using the library.  And of course check in to see if there's any updates :)

Coordinator
Aug 2, 2011 at 3:29 AM

You should definitely look into the FP stuff. The xlllapack project requires Intel's MKL, but you can use that to call the linear algebra routines that the heavy hitters use for numerical computing. The xllgsl library provides similar functionality out of the box, but you aren't going to be able to link your Excel code to a library that is optimally tuned to the CPU that you want to run on.

Keep it coming. You are posing very challenging problems that even I am surprised are possible to solve using an Excel add-in.

Coordinator
Aug 2, 2011 at 12:06 PM

I hope you don't think I am being presumptuous, but you are the first person that has ever found and fixed a bug in any open source library I've created. The usual 'contribution' is more along the lines of "Your shit doesn't work on my computer, man."

I've added you as a developer and set up a branch for you at https://nxll.svn.codeplex.com/svn/branches/colinrodriguez. If you don't know subversion, you should spend 30 minutes getting familiar with it. Check out your branch, commit changes to your heart's content, and when we are both happy I can fold it back into the main trunk so everyone can benefit from your handiwork.

Developer
Aug 2, 2011 at 9:40 PM

Haha, thanks for the vote of confidence in my less than polished C++ skills :P  Just trying to be helpful if you went through the trouble of releasing this as open source!

But as a heads up, I got pulled into this Excel addon project briefly to help out a couple of mathematicians who were ok writing a little scientific computing code, but didn't want to deal with setting up the Excel->XLL bridge.  So now that the boilerplate is mostly taken care of, I'm probably only going to be pulled back in by them mostly when things aren't working or they need some more help.  But as far as I do any future bug hunting or useful utility creation, I'm definitely more than happy to try and integrate it back into the project :)

Developer
Aug 2, 2011 at 9:43 PM

Also, on a side note, I did a tour of linear algebra packages a while back, and did look at MKL but the one I eventually settled on that is way easier to work with and in my brief benchmarking amazingly outperformed the "standard" packages like LAPACK/MKL is actually an open source project called Eigen (http://eigen.tuxfamily.org/).  And despite being released under the LGPL, its safe for use in commercial work because its a headers only library.  Definitely check it out if you do a lot of numerical computation!

Coordinator
Aug 3, 2011 at 12:18 AM

I've heard good things about eigen, but haven't used it. It looks like it has vector and matrix classes that force you to copy data over to their side of the fence. For best performance in Excel you should just hand the double* pointer over from the FP data type without doing a copy.

The latest MKL is pretty smooth to use, unlike earlier versions. I use it in xllblas and xlllapack. The only thing you need to do is select a dropdown for which library to use: sequenctial, parallel, or cluster. That's it. One of my clients wanted to do a 1000 x 1000 Cholesky decomposition. Before telling him that was impossible to do in Excel, I tried it. It worked just fine. 

Glad you were successful in solving those guy's problem. I'm a mathematician by training too.

Developer
Aug 3, 2011 at 6:47 PM

Haha... yeah, its not that obvious from their initial docs, but Eigen does actually let you treat preexisting memory blocks like matrixes using their Map class without all the overhead of having to create a copy... so perfect for just using the double* from the FP type.  Mostly I was sold on Eigen by the fact it had comparable runtimes and its template magic let you write C = A * B; to multiply matrixes rather than a 13 parameter dgemm function call!

If you don't mind, I did have another quick question question.  Have you found any way to effectively have optional parameters using the FP type?  On my first pass before learning about the FP type, I had set up everything using LPXLOPERX's and then Excel would simply pass instances of xltypeMissing where an optional parameter wasn't used which I could check for and act accordingly.  Apparently the overhead of converting an LOPERX xltypeMulti into a usable matrix is slowing things down more than desired, but we would like to maintain the flexibility of optional parameters if possible.  I know we want to have our cake and eat it too, but is there any way you have come across of having optional parameters of FP type?  I sure couldn't figure out how to do it...

Coordinator
Aug 3, 2011 at 7:51 PM

Let's be fair, dgemm only has 12 parameters. :-). The parameters are there for a reason though. Eventually you will get to the point where you need them.

Glad eigen lets you map the memory. It definitely looks like it was somethng written by smart programmers.

Short answer: FPs are not allowed to be missing. You can put in a 0 argument and get a 1x1 FP with array value 0.
Longer answer: You can wrap them up in a handle (type HANDLEX, which is currently just a double), declare them as XLL_HANDLEX (== XLL_DOUBLEX), and when they are missing you get 0 handed to you by Excel. See http://xllarray.codeplex.com for examples. Start with xllarray/getset.cpp. There is a notion of an empty array that let's the other functions handle boundary cases smoothly.

You also might be interested in http://xllfunctional.codeplex.com. It is a start at first class functions for Excel. For example, you can curry user defined functions using it. E.g., if F takes 3 arguments you can do this FUNCTIONAL.CALL(FUNCTIONAL.BIND(F, 1,  , 3), 2) and get back F(1, 2, 3). The missing arguments will be the arguments for the bound function.

Developer
Aug 4, 2011 at 1:14 AM

Haha, yeah... you can wind up specifying those 12! parameters with Eigen too, but its all very OO and you usually don't need to... good times.

But thanks for the extra info!  I was contemplating some solution along the lines of the HANDLE approach, maybe not right now but some future date if people still aren't happy about the setup, so I'll definitely take a look!  Really wish Microsoft had built in real support for optional parameters, but that's neither here nor there at this point...

And nice; very bold trying to implement some functional programming in Excel :P