Skip Menu |

This queue is for tickets about the Math-BigInt-FastCalc CPAN distribution.

Report information
The Basics
Id: 25033
Status: resolved
Worked: 10 min
Priority: 0/
Queue: Math-BigInt-FastCalc
People
Owner: Nobody in particular
Requestors: mschwern [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Important
Broken in: 0.11
Fixed in: (no value)

History Show all quoted text
  Mon Feb 19 10:35:26 2007 mschwern [...] cpan.org - Ticket created
Subject: It leaks
Math::BigInt::FastCalc causes Math::BigFloat to leak memory. Observe. $ perl -e 'use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) }' & [1] 3069 $ ps auwwwx | head USER PID %CPU %MEM VSZ RSS TT STAT STARTED TIME COMMAND $ ps auwwwx | grep 3069 schwern 3069 100.0 -1.1 49200 23836 p2 R 10:32AM 0:07.65 perl -e use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) } $ ps auwwwx | grep 3069 schwern 3069 98.7 -1.4 50244 30116 p2 R 10:32AM 0:10.06 perl -e use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) } $ ps auwwwx | grep 3069 schwern 3069 98.8 -1.7 59512 34644 p2 R 10:32AM 0:11.80 perl -e use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) } And now without FastCalc. $ perl -wle 'use Devel::Hide qw(Math::BigInt::FastCalc); use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) }' & [1] 3137 $ ps auwwwx | grep 3137 schwern 3137 98.4 -0.2 30632 4144 p2 R 10:33AM 0:05.44 perl -wle use Devel::Hide qw(Math::BigInt::FastCalc); use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) } $ ps auwwwx | grep 3137 schwern 3137 100.0 -0.2 30632 4144 p2 R 10:33AM 0:06.86 perl -wle use Devel::Hide qw(Math::BigInt::FastCalc); use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) } $ ps auwwwx | grep 3137 schwern 3137 99.4 -0.2 30632 4144 p2 R 10:33AM 0:08.48 perl -wle use Devel::Hide qw(Math::BigInt::FastCalc); use Math::BigFloat; while (1) { $foo = Math::BigFloat->new(0) }
  Tue Feb 20 22:34:00 2007 mschwern [...] cpan.org - Correspondence added
From: MSCHWERN [...] cpan.org
It seems it only leaks for 0. while (1) { $foo = Math::BigFloat->new(1) } Does not leak.
  Tue Feb 20 23:00:41 2007 mschwern [...] cpan.org - Correspondence added
On Tue Feb 20 22:34:00 2007, MSCHWERN wrote: Show quoted text
> It seems it only leaks for 0. > > while (1) { $foo = Math::BigFloat->new(1) } > > Does not leak.
I suspect this code of being the reason why 0 leaks and 1 does not. Math::BigFloat::new(/usr/local/perl/5.8.8/lib/Math/BigFloat.pm:212): 212: $self->{sign} = '+', $self->{_e} = $MBI->_one() 213: if $$miv eq '0' and $$mfv eq ''; As its a special case for 0 which does not exist for 1. Calling Math::BigInt::FastCalc->_one, _zero, _two or _ten causes the leak. $ perl -wle 'use Math::BigFloat; while (1) { $foo = Math::BigInt::FastCalc->_ten }' & [1] 2287 $ ps aux | grep 2287 schwern 2287 98.7 -16.3 361436 342836 p2 R 7:59PM 0:04.04 perl -wle $ ps aux | grep 2287 schwern 2287 97.8 -19.9 439756 417500 p2 R 7:59PM 0:04.93 perl -wle $ ps aux | grep 2287 schwern 2287 97.1 -22.8 497452 478164 p2 R 7:59PM 0:05.66 perl -wle
  Tue Feb 20 23:00:43 2007 mschwern [...] cpan.org - Status changed from 'new' to 'open'
  Tue Feb 20 23:10:19 2007 mschwern [...] cpan.org - Correspondence added
$ perl -wle 'use Devel::Peek; use Math::BigInt::FastCalc; $foo = Math::BigInt::FastCalc->_zero; print Dump $foo' SV = RV(0x1819ea4) at 0x1801b70 REFCNT = 1 FLAGS = (ROK) RV = 0x1801180 SV = PVAV(0x1806268) at 0x1801180 REFCNT = 2 FLAGS = () IV = 0 NV = 0 ARRAY = 0x401ae0 FILL = 0 MAX = 3 ARYLEN = 0x0 FLAGS = (REAL) Elt No. 0 SV = IV(0x18103d4) at 0x18012c4 REFCNT = 1 FLAGS = (IOK,pIOK) IV = 0 That PVAV should have a ref count of 1. Compare with the result from Calc. $ perl -wle 'use Devel::Peek; use Math::BigInt; $foo = Math::BigInt::Calc->_zero; print Dump $foo' SV = RV(0x181a044) at 0x1801b70 REFCNT = 1 FLAGS = (ROK) RV = 0x1801180 SV = PVAV(0x1806268) at 0x1801180 REFCNT = 1 FLAGS = () IV = 0 NV = 0 ARRAY = 0x401ad0 FILL = 0 MAX = 0 ARYLEN = 0x0 FLAGS = (REAL) Elt No. 0 SV = IV(0x1862f04) at 0x18012c4 REFCNT = 1 FLAGS = (IOK,pIOK) IV = 0
  Tue Feb 20 23:21:53 2007 mschwern [...] cpan.org - Correspondence added
Attached is a test. If you change the base class of the Leaker to be Calc instead of FastCalc you can see it passes fine.
#!/usr/bin/perl -w use Test::More tests => 4; use strict; # Test for memory leaks. package Math::BigInt::FastCalc::LeakCheck; use base qw(Math::BigInt::FastCalc); my $destroyed = 0; sub DESTROY { $destroyed++ } package main; for my $method (qw(_zero _one _two _ten)) { $destroyed = 0; { my $num = Math::BigInt::FastCalc::LeakCheck->$method(); bless $num, "Math::BigInt::FastCalc::LeakCheck"; } is $destroyed, 1, "$method does not leak memory"; }
  Wed Feb 21 00:06:51 2007 mschwern [...] cpan.org - Correspondence added
Subject: [PATCH] It leaks
Attached is a patch to fix the memory leak in _zero, _one, _two and _ten. The problem was three fold. 1) The AV reference was not made mortal before returning it. 2) You have to declare how many arguments you're going to return via XSRETURN(). 3) If you're going to be doing direct stack manipulation I believe you should be using PPCODE. See "Returning SVs, AVs and HVs through RETVAL" in perlxs for details. The attached patch makes sure to mortalize the AV, that takes care of 1. It also uses the much simpler CODE/OUTPUT blocks with RETVAL to allow Perl to do the stack managing. That takes care of 2 and 3. The desire to do stack manipulation yourself might have been for optimization purposes but I see no performance difference running the tests for the attached patch and using PPCODE with direct stack manipulation. See the CONSTANT_OBJ_using_PPCODE.patch for the alternative. Looking at the rest of the code I suspect there are many many more leaks of this nature.
--- FastCalc.xs 2007-02-20 21:02:31.000000000 -0800 +++ FastCalc.xs.new 2007-02-20 21:00:06.000000000 -0800 @@ -231,43 +231,31 @@ ############################################################################## #define CONSTANT_OBJ(int) \ - RETVAL = newAV(); \ - sv_2mortal((SV*)RETVAL); \ - av_push (RETVAL, newSViv( int )); \ + ST(0) = newAV(); \ + av_push(ST(0), newSViv( int )); \ + sv_2mortal((SV*) ST(0)); \ + ST(0) = newRV_noinc(ST(0)); \ + XSRETURN(1); -AV * +void _zero(class) - CODE: + PPCODE: CONSTANT_OBJ(0) - OUTPUT: - RETVAL - -############################################################################## -AV * +void _one(class) - CODE: + PPCODE: CONSTANT_OBJ(1) - OUTPUT: - RETVAL - -############################################################################## -AV * +void _two(class) - CODE: + PPCODE: CONSTANT_OBJ(2) - OUTPUT: - RETVAL -############################################################################## - -AV * +void _ten(class) - CODE: + PPCODE: CONSTANT_OBJ(10) - OUTPUT: - RETVAL ##############################################################################
Auto-merging (0, 27433) /local/Math-BigInt-FastCalc to /vendor/Math-BigInt-FastCalc (base /vendor/Math-BigInt-FastCalc:27431). A t/leak.t U FastCalc.xs ==== Patch <-> level 1 Source: 9c88509d-e914-0410-b01c-b9530614cbfe:/local/Math-BigInt-FastCalc:27433 Target: 9c88509d-e914-0410-b01c-b9530614cbfe:/vendor/Math-BigInt-FastCalc:27431 Log: r27432@windhund: schwern | 2007-02-20 20:12:29 -0800 Local copy r27433@windhund: schwern | 2007-02-20 20:46:46 -0800 Fix the memory leak in _zero, _one, _two and _ten. I suspect the whole module leaks like this. === t/leak.t ================================================================== --- t/leak.t (revision 27431) +++ t/leak.t (patch - level 1) @@ -0,0 +1,24 @@ +#!/usr/bin/perl -w + +use Test::More tests => 4; +use strict; + +# Test for memory leaks. + +package Math::BigInt::FastCalc::LeakCheck; +use base qw(Math::BigInt::FastCalc); + +my $destroyed = 0; +sub DESTROY { $destroyed++ } + + +package main; + +for my $method (qw(_zero _one _two _ten)) { + $destroyed = 0; + { + my $num = Math::BigInt::FastCalc::LeakCheck->$method(); + bless $num, "Math::BigInt::FastCalc::LeakCheck"; + } + is $destroyed, 1, "$method does not leak memory"; +} === FastCalc.xs ================================================================== --- FastCalc.xs (revision 27431) +++ FastCalc.xs (patch - level 1) @@ -230,51 +230,44 @@ ############################################################################## -void -_zero(class) - INIT: - AV* a; +#define CONSTANT_OBJ(int) \ + RETVAL = newAV(); \ + sv_2mortal((SV*)RETVAL); \ + av_push (RETVAL, newSViv( int )); \ +AV * +_zero(class) CODE: - a = newAV(); - av_push (a, newSViv( 0 )); /* zero */ - ST(0) = newRV_noinc((SV*) a); + CONSTANT_OBJ(0) + OUTPUT: + RETVAL ############################################################################## -void +AV * _one(class) - INIT: - AV* a; - CODE: - a = newAV(); - av_push (a, newSViv( 1 )); /* one */ - ST(0) = newRV_noinc((SV*) a); + CONSTANT_OBJ(1) + OUTPUT: + RETVAL ############################################################################## -void +AV * _two(class) - INIT: - AV* a; - CODE: - a = newAV(); - av_push (a, newSViv( 2 )); /* two */ - ST(0) = newRV_noinc((SV*) a); + CONSTANT_OBJ(2) + OUTPUT: + RETVAL ############################################################################## -void +AV * _ten(class) - INIT: - AV* a; - CODE: - a = newAV(); - av_push (a, newSViv( 10 )); /* ten */ - ST(0) = newRV_noinc((SV*) a); + CONSTANT_OBJ(10) + OUTPUT: + RETVAL ############################################################################## ==== BEGIN SVK PATCH BLOCK ==== Version: svk v2.0.0 (darwin) eJyNlV9v2zYQwP2sx27AXtlU3ew2ikVKsi0FNZwmdtemTdrGNTYgq6A/51iILLoSbTcri9UO0hX7 HNvLMOxlz/sc28fZSaqHBFuaCIRMmXe/u+Mdj720v9mhst3WpUp1eTDYdZynnghGt6kp1YaEMBI8 VS0Zwwxi1ZAxP1JNmXhjwNWMT9MgnwgvPQKRT6LgGES7TRHXKnHdArHCFlTfEzzJVLvAuyIFUKk0 O5bsGPlwVWrLDHClwLopzKIs4gm6wZqmYaAIylPU5xNI3JRz8e8Sy7V1GcQ8AzfH50grl2cqhlQo hFEKAfp0gv+KgrTSLgTNFXgYxegYlT0vE9teHGy8zi5IF55aH22dh1J0o9PIWYbakl4YlqiWFPUY vOON3ChzVSY1ejmElZBi1ZtM4hNXwGsRQiy8HOy6qmE2cYv3KpXKKTxb/PB797Oful/82dM+DF7+ vLuonT3q/9jd/vXZoraz/Ly3vLGzpDuL+Mz/bvlgwZe95Zc7y1dn3WXr1Hu5wK9Fd/nVEtXef73g H7pPHvTObt2sT7O07kdJfQJpTLS5okwzIH3IhOM84SkQgdOM3GsTczMTaRSITUW5RYY8JWMYYxwk jzfbUJSJFxx7R0CeeGLkOPejo4cJMlY76ziPUW57BMGx76GFV/NqDUnjE6KGaCHlJxCSe0TfVLKp T3a6B/3n+9+SN3fvkrfK2IsSlEXRMYgRD0kVtd3vIeXE5QkQV8xxJiCp1cgbhRCSTMfkntau1vwY smydrK29jfCH4izkkJGEi7VN5a3SMc/XUlkMmFCDSbvF7GYTbKsBoY3D95nPrKFFLc/WLb9Iv3VZ 5syc4JuG7Vst3dJt1mzafgv8hk+pYYWB5fu2oVp6q8zt38//Olw73av89k3l3YJXHr6vvHt07r2b vyu/3PijcyuEYYTxbu/vHfS39vru/v1H1SgRNXLxOcz3gDzv9gdbj3FPE5hvDXC3yX+fUjKbuQxT Kby4Wj0Y3KmVmhcVSklv5k6m2YhUS5H1nH0wiGZVgm6Q2keVQ0XZGpA7SpGiahB7WVZD9e39na5T YC4EoOdr+y/6T1/0nXOOY5XlzwqFeb6SRK9FwmK5ksSuR4Lkap8uDe9/i68oHQq6bwYWNXysHQgZ tUxoDAPGhkAtw6NFi2JMlyTN2xTrzKMkHE2T0MFcBqM5pAmRhOl6U9OZxnScOpQ5zCaa3tJ1hTzm gReTgE9OlBJhXAdhNnCsEL3oNREjON8EsAZIkfL14liul+fSS8LibG4Q8pBk02yC7a/QnI94DAoZ 83AaQ9lFSBwdY8cZRdhPMEit3WYSAy1vmX5xVTjOiySaQZp58W3sv6acYLvBWwtvEPyYTqNQZU1Z j/MI63kr0spOpK0a0aq3q0ZD2kGrhecz1MCmpqabVNd8nQaab1uG3qBm4A+hXWPyUkstWZ9BEvL0 U6bodU2pTf1acs4nwnOK4P4BOLZbog== ==== END SVK PATCH BLOCK ====
  Sat Mar 03 10:23:15 2007 TELS [...] cpan.org - Correspondence added 5 min
On Wed Feb 21 00:06:51 2007, MSCHWERN wrote: Show quoted text
> Attached is a patch to fix the memory leak in _zero, _one, _two and _ten. > > The problem was three fold.
[snip] Thanx for the explanation and patchs. Will apply them soon. The reason for the dogy code and leaks is that I have basically zero knowledge of XS, all I did was copy & paste some stuff together until it "worked". :) All the best, Tels
  Sat Mar 03 12:22:51 2007 TELS [...] cpan.org - Correspondence added 5 min
Fixed in release 0.12 to CPAN and applied as patch #30454 to blead. Thank you a lot for you report! :)
  Sat Mar 03 12:22:55 2007 TELS [...] cpan.org - Status changed from 'open' to 'resolved'