Issue 73457 - Memory Leak in all Basic type void Method calls
Summary: Memory Leak in all Basic type void Method calls
Status: CLOSED FIXED
Alias: None
Product: App Dev
Classification: Unclassified
Component: api (show other issues)
Version: 3.3.0 or older (OOo)
Hardware: All All
: P3 Trivial
Target Milestone: ---
Assignee: joerg.skottke
QA Contact: issues@api
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-14 13:46 UTC by terrye
Modified: 2013-02-24 21:06 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
Bug Analysis Working Paper (35.65 KB, application/vnd.oasis.opendocument.text)
2007-02-22 00:16 UTC, terrye
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description terrye 2007-01-14 13:46:06 UTC
Every basic subroutine call leaks approximately 216+24N bytes of memory where N
is the number of arguments in the call *IF* N>0.  The following code fragment
demonstrates this issue.
 
Sub Main
for i = 1 to 100000: X(a,b): Next i
End Sub
Sub X(n,m)
'X=0 or even a=X
End Sub

There is no leak if the call is a function call (which sets the return value or
there are no arguments.  This happens on both the Linux and Windows builds.
2.0.3, 2.0.4 and 2.1 

The issue seems to be related to referencing PARAM 0 within the subroutine /
function if the parameter array has been contructed following an ARGC
instruction.  **Any** PARAM 0 instruction executed within the routine (even in
the context of an RVALUE) triggers correct garbage collection of the argument
array following the LEAVE.

This memory is not reclaimed on document closure.  In the case of Windows you
need to exit entirely and disable quickstarter.

I'll try to track it down a bit further, but this will need stepping through
with the debugger.  I've been aware of a bad memory leak that was occuring basic
macro execution, and this will be an important one to fix.
Comment 1 kpalagin 2007-01-14 17:23:39 UTC
Confirming with OO 2.1 on WinXP - mem usage by soffice.bin jumps by 26MB with 
each macro run.
Setting priority to P2.
Comment 2 jsc 2007-01-15 08:35:25 UTC
jsc -> ab: can you please check this, it seems to be a serious problem.
Comment 3 ab 2007-01-15 08:48:31 UTC
Started

I will have a look. I'm not sure if I can fix this for OOo 2.2 due to
limited time and other important tasks, but I will try. Nevertheless
I don't agree with P2. P2 is only justified for huge memory leaks
and within a programming language you can push any leak as 
huge as you like by just increasing the loop count.

As this is no regression either, setting to P3, OOo 2.3 for now.
If there is any time left, this task will have top priority and I will 
change it to OOo 2.2 then.
Comment 4 bmarcelly 2007-01-15 09:37:44 UTC
--> ab
This leakage case is important because effects are cumulative, you have to stop 
OOo to release memory. 

Quick Starter is active by default on MS-Windows, and most users don't change 
this.
Some people use OpenOffice as a server for converting to PDF or to other 
formats with Basic macros, and complain of crash or blocking after some time, 
so they need to kill OOo periodically.

terrye has found an obvious case of leakage, it should be corrected. This is 
more useful than yet another feature.


Comment 5 terrye 2007-01-18 02:16:28 UTC
I've created a comparison test case, and hooked in the Ref Class series
QueryDelete() method with backtraces to work out exactly where the code
divergence is occuring.  I need to recompile a few modules -O0 to make debugging
and stepping easier, but hopefully I will be able to give you the exact bug and
code fix by the  weekend.  //Terry
Comment 6 terrye 2007-01-28 21:24:24 UTC
If A is a subroutine, then the method call A(0) generates the Pcode statements
FIND A, Empty, Args
GET
The StepFIND(“Aâ€, Empty!Args) instruction call invokes SbInstance::FindElement
which after a lot of cascade logic ends up the code branch which processes
Methods. This invokes the following statement to make the call:
	SbxVariable* pNew = new SbxMethod( *((SbxMethod*)pElem) );
which cascades through the copy constructor for SbxVariable, after calling the
copy constructors for the superclass SbxValue and the properties pPar and pInfo,
this goes branch depending on where the value is readable (which it isn’t for a
method call) and then issues a Broadcast(SBX_HINT_DATAWANTED).   Broadcast is a
virtual method on since this variable is a SbMethod, SbMethod::Broadcast is
invoked.  It is this Broadcast that is ultimately invokes the Run method which
executes the method.  

This call cascade generates the class property SbxArray refArgv created in
StepARGC() to hold the Argument Stack, the SbxVariable p created in LOADI() to
hold the constant 0 used as the parameter, and the copy constructor for SbMethod
creates a new SbMethod in. In a normal function call all three of these
variables are ultimately destroyed and garbage collected in the various exit
sequences as the routine unroll through the call stack.   The first to go is the
SbxArray that was referenced by refArgv.  The StepPARAM() routine fetches the
parameter with a p = refParams->Get( 0 ) and skips some processing handling
SbxERROR and coercion of types, falling though to SetupArgs(p, nOp1) which does
a p->SetParameters (NULL) if it is not a ByVal.  This in turn does a
SbxArrayRef::operator= assignment to NULL, which dereferences the previous
contents and this triggers the SvRefBase garbage collection process of the now
zero reference SbxArray.  The SbMethod is destroyed later in SbMethod::Broadcast
by the assignment pCst = NULL which invokes the SbMethod:: operator= which
dereferences the underlying SbMethod, and finally the SbxVariable is referenced
and deleted in pushing the return value.

However all of this logic is built on the assumption that the method actually
returns a value. In the case of a standard Sub call, the method type is SbxVOID
so this assumption is flawed.  The logic flow that is folloed in this case
short-circuits code paths that dereference the values. I’ve verified this by not
setting the return value on a function call and force a return on a subroutine
call.  

The challenge is no longer understanding why the leak occurs but more what is
the appropriate fix to remove it.  My problem is that this whole code area seems
to have fix-ups upon fix-ups. I’ve done this in my test harness to validate the
diagnosis, but I don’t really want to propose another botch.  Need to think
about this one a bit more.
Comment 7 terrye 2007-02-02 13:35:13 UTC
Almost there.  This one has taken more effort to get to the bottom of than I
though.  Sorry.  One of the consequences of this diagnosis is that I've realised
that the leak is a consequence of the assumption embedded in the code that all
method calls return a value.  Hence the leak occurs for ANY method call which
doesn't.  So if you replace the above For loop to a call to c.setValue(1) where
c is ThisComponent.Sheets.Sheet1.getCellByPosition(0,0) for example.

This extends the scope and impact of the leak to any basic code which is doing a
lot of .setXXXX calls.  Hence my recommendation to raise this to a P2.  However,
I hope to have the code fix finalised this weekend.  //Terry
Comment 8 terrye 2007-02-10 12:44:14 UTC
*** FIX ***

The minimum impact change which removes this leak is a one line change to
sbxmod.cxx:
@@ -2057,7 +2057,8 @@
                if( mpPar.Is() )
         {
                        // this, als Element 0 eintragen, aber den Parent nicht
umsetzen!
-               mphoPar->PutDirect( pThisCopy, 0 );
+               if (GetType() != SbxVOID) mpPar->PutDirect( pThisCopy, 0 );
                        SetParameters( NULL );
         }

This change should be put in ASAP in 2.3 or 2.4, as it will close a major memory
leak in all applications which make heavy use of Basic.

Note that this whole code area has a number of implementation weaknesses that
impact heavily on runtime performance.  I've drafted a paper on this and sent to
AB for comment. I will attach it as background once AB has had a chance to comment.
Comment 9 terrye 2007-02-10 12:54:32 UTC
By the way, the diff header line should read @@ -2057,7 +2057,7 @@

I removed a copy of my audit comment and forget to change the line count.  Also
the line number 2057 refers to the ooo-build 2.0.4 variant.  Sorry.  //Terry  
Comment 10 terrye 2007-02-22 00:16:26 UTC
Created attachment 43254 [details]
Bug Analysis Working Paper
Comment 11 ab 2007-03-12 13:19:16 UTC
ab->Terry: Thanks for your great analysis. I've checked in your
"minimum impact patch" now.

-> FIXED
Comment 12 kpalagin 2007-04-29 07:14:01 UTC
I remember seeing the idea of code refactoring to improve performance, but 
can't find anymore in this discussion.
Do we have issue for that?
Comment 13 terrye 2007-05-06 01:48:57 UTC
I've done quite a lot of work on the analysis here, but it's the old problem --
finding the hours in the day to move this one forward.   If you feel that we
should make this publicly available then we should open another issue, but this
doesn't relate to 73457 -- which was a specific leak and bug fix.
Comment 14 kpalagin 2007-05-06 11:09:46 UTC
terrye,
I would very much like to see OpenOffice to have better performance.
If you see potential for improvement please file new issue - I am sure 
community will support new RFE.
Thank you very much!
WBR
Kirill Palagin.
Comment 15 ab 2007-07-26 09:50:27 UTC
ab->jsk: To verify just run the test code some times. With the ab34 Office
memorey usage before and after the run should be nearly the same,
whereas in the m222 master the memery use increases significantly
with every run.
Comment 16 joerg.skottke 2007-08-02 14:29:34 UTC
The office (soffice.bin) stays at 15656KB after several runs of the testcode.
Veriffied.
Comment 17 joerg.skottke 2007-11-01 09:57:03 UTC
Rechecked in OOG680m7, closing