Anomie has uploaded a new change for review.
https://gerrit.wikimedia.org/r/93073
Change subject: LuaStandalone: Release functions when no longer referenced
......................................................................
LuaStandalone: Release functions when no longer referenced
The LuaStandalone interpreter needs to keep a mapping from integers
returned to PHP to the corresponding function. But if it never releases
these functions when PHP no longer has any reference to them, it can
result in Lua running out of memory if a module with a large number of
functions is invoked many times in one page.
The fix here is to track which function ids are referenced from PHP, and
periodically send the list to Lua so it can remove any that are no
longer used from its cache.
This also takes care of another issue where having multiple interpreter
instances and passing function objects from one into another could call
the wrong function in Lua.
Bug: 51886
Change-Id: I4f15841051f7748d1d6df24080949e5cbd88f217
---
M engines/LuaStandalone/LuaStandaloneEngine.php
M engines/LuaStandalone/MWServer.lua
M engines/LuaStandalone/mw_main.lua
M engines/LuaStandalone/protocol.txt
M tests/engines/LuaStandalone/LuaStandaloneInterpreterTest.php
5 files changed, 204 insertions(+), 12 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Scribunto
refs/changes/73/93073/1
diff --git a/engines/LuaStandalone/LuaStandaloneEngine.php
b/engines/LuaStandalone/LuaStandaloneEngine.php
index 4fea92a..e22761a 100644
--- a/engines/LuaStandalone/LuaStandaloneEngine.php
+++ b/engines/LuaStandalone/LuaStandaloneEngine.php
@@ -77,9 +77,12 @@
}
class Scribunto_LuaStandaloneInterpreter extends Scribunto_LuaInterpreter {
- var $engine, $enableDebug, $proc, $writePipe, $readPipe, $exitError;
+ static $nextInterpreterId = 0;
+ var $engine, $enableDebug, $proc, $writePipe, $readPipe, $exitError,
$id;
function __construct( $engine, $options ) {
+ $this->id = self::$nextInterpreterId++;
+
if ( $options['errorFile'] === null ) {
$options['errorFile'] = wfGetNull();
}
@@ -122,7 +125,9 @@
$cmd = wfEscapeShellArg(
$options['luaPath'],
dirname( __FILE__ ) . '/mw_main.lua',
- dirname( dirname( dirname( __FILE__ ) ) ) );
+ dirname( dirname( dirname( __FILE__ ) ) ),
+ $this->id
+ );
if ( php_uname( 's' ) == 'Linux' ) {
// Limit memory and CPU
$cmd = wfEscapeShellArg(
@@ -231,21 +236,28 @@
}
public function loadString( $text, $chunkName ) {
+ $this->cleanupLuaChunks();
+
$result = $this->dispatch( array(
'op' => 'loadString',
'text' => $text,
'chunkName' => $chunkName
) );
- return new Scribunto_LuaStandaloneInterpreterFunction(
$result[1] );
+ return new Scribunto_LuaStandaloneInterpreterFunction(
$this->id, $result[1] );
}
public function callFunction( $func /* ... */ ) {
if ( !($func instanceof
Scribunto_LuaStandaloneInterpreterFunction) ) {
throw new MWException( __METHOD__.': invalid function
type' );
}
+ if ( $func->interpreterId !== $this->id ) {
+ throw new MWException( __METHOD__.': function belongs
to a different interpreter' );
+ }
$args = func_get_args();
unset( $args[0] );
// $args is now conveniently a 1-based array, as required by
the Lua server
+
+ $this->cleanupLuaChunks();
$result = $this->dispatch( array(
'op' => 'call',
@@ -264,6 +276,16 @@
'op' => 'wrapPhpFunction',
'id' => $id ) );
return $ret[1];
+ }
+
+ public function cleanupLuaChunks() {
+ if ( isset(
Scribunto_LuaStandaloneInterpreterFunction::$anyChunksDestroyed[$this->id] ) ) {
+ unset(
Scribunto_LuaStandaloneInterpreterFunction::$anyChunksDestroyed[$this->id] );
+ $this->dispatch( array(
+ 'op' => 'cleanupChunks',
+ 'ids' =>
Scribunto_LuaStandaloneInterpreterFunction::$activeChunkIds[$this->id]
+ ) );
+ }
}
public function isLuaFunction( $object ) {
@@ -472,11 +494,13 @@
$s .= '}';
return $s;
case 'object':
- if ( $var instanceof
Scribunto_LuaStandaloneInterpreterFunction ) {
- return 'chunks[' . intval( $var->id )
. ']';
- } else {
+ if ( !( $var instanceof
Scribunto_LuaStandaloneInterpreterFunction ) ) {
throw new MWException( __METHOD__.':
unable to convert object of type ' .
get_class( $var ) );
+ } elseif ( $var->interpreterId !== $this->id ) {
+ throw new MWException( __METHOD__.':
unable to convert function belonging to a different interpreter' );
+ } else {
+ return 'chunks[' . intval( $var->id )
. ']';
}
case 'resource':
throw new MWException( __METHOD__.': unable to
convert resource' );
@@ -540,9 +564,47 @@
}
class Scribunto_LuaStandaloneInterpreterFunction {
- public $id;
+ public static $anyChunksDestroyed = array();
+ public static $activeChunkIds = array();
- function __construct( $id ) {
+ public $interpreterId, $id;
+
+ function __construct( $interpreterId, $id ) {
+ $this->interpreterId = $interpreterId;
$this->id = $id;
+ $this->incrementRefCount();
+ }
+
+ function __clone() {
+ $this->incrementRefCount();
+ }
+
+ function __wakeup() {
+ $this->incrementRefCount();
+ }
+
+ function __destruct() {
+ $this->decrementRefCount();
+ }
+
+ private function incrementRefCount() {
+ if ( !isset( self::$activeChunkIds[$this->interpreterId] ) ) {
+ self::$activeChunkIds[$this->interpreterId] = array(
$this->id => 1 );
+ } elseif ( !isset(
self::$activeChunkIds[$this->interpreterId][$this->id] ) ) {
+ self::$activeChunkIds[$this->interpreterId][$this->id]
= 1;
+ } else {
+
self::$activeChunkIds[$this->interpreterId][$this->id]++;
+ }
+ }
+
+ private function decrementRefCount() {
+ if ( isset(
self::$activeChunkIds[$this->interpreterId][$this->id] ) ) {
+ if (
--self::$activeChunkIds[$this->interpreterId][$this->id] <= 0 ) {
+ unset(
self::$activeChunkIds[$this->interpreterId][$this->id] );
+ self::$anyChunksDestroyed[$this->interpreterId]
= true;
+ }
+ } else {
+ self::$anyChunksDestroyed[$this->interpreterId] = true;
+ }
}
}
diff --git a/engines/LuaStandalone/MWServer.lua
b/engines/LuaStandalone/MWServer.lua
index adb95a8..0361ca7 100644
--- a/engines/LuaStandalone/MWServer.lua
+++ b/engines/LuaStandalone/MWServer.lua
@@ -1,8 +1,15 @@
MWServer = {}
--- Create a new MWServer object
-function MWServer:new()
+function MWServer:new( interpreterId )
+ interpreterId = tonumber( interpreterId )
+ if not interpreterId then
+ error( "bad argument #1 to 'MWServer:new' (must be a number or
convertible to a number)", 2 )
+ end
+
obj = {
+ interpreterId = interpreterId,
+ nextChunkId = 1,
chunks = {},
xchunks = {},
protectedFunctions = {},
@@ -70,6 +77,13 @@
-- @param message The message from PHP
-- @return A response message to send back to PHP
function MWServer:handleCall( message )
+ if not self.chunks[message.id] then
+ return {
+ op = 'error',
+ value = 'function id ' .. message.id .. ' does not
exist'
+ }
+ end
+
local n, result = self:listToCountAndTable( xpcall(
function ()
return self.chunks[message.id]( unpack( message.args,
1, message.nargs ) )
@@ -151,10 +165,31 @@
-- @param chunk The function value
-- @return The chunk ID
function MWServer:addChunk( chunk )
- local id = #self.chunks + 1
+ local id = self.nextChunkId
+ self.nextChunkId = id + 1
self.chunks[id] = chunk
self.xchunks[chunk] = id
return id
+end
+
+--- Handle a "cleanupChunks" message from PHP.
+-- Remove any chunks no longer referenced by PHP code.
+--
+-- @param message The message from PHP
+-- @return A response message to send back to PHP
+function MWServer:handleCleanupChunks( message )
+ for id, chunk in pairs( self.chunks ) do
+ if not message.ids[id] then
+ self.chunks[id] = nil
+ self.xchunks[chunk] = nil
+ end
+ end
+
+ return {
+ op = 'return',
+ nvalues = 0,
+ values = {}
+ }
end
--- Handle a "registerLibrary" message from PHP.
@@ -273,6 +308,9 @@
self:sendMessage( msgToPhp )
elseif op == 'wrapPhpFunction' then
msgToPhp = self:handleWrapPhpFunction( msgFromPhp )
+ self:sendMessage( msgToPhp )
+ elseif op == 'cleanupChunks' then
+ msgToPhp = self:handleCleanupChunks( msgFromPhp )
self:sendMessage( msgToPhp )
elseif op == 'getStatus' then
msgToPhp = self:handleGetStatus( msgFromPhp )
@@ -461,7 +499,8 @@
else
id = self:addChunk(var)
end
- return
'O:42:"Scribunto_LuaStandaloneInterpreterFunction":1:{s:2:"id";i:' .. id .. ';}'
+ return
'O:42:"Scribunto_LuaStandaloneInterpreterFunction":2:{s:13:"interpreterId";i:'
..
+ self.interpreterId .. ';s:2:"id";i:' .. id ..
';}'
elseif t == 'thread' then
error("Cannot pass thread to PHP")
elseif t == 'userdata' then
diff --git a/engines/LuaStandalone/mw_main.lua
b/engines/LuaStandalone/mw_main.lua
index 22fa782..30084fc 100644
--- a/engines/LuaStandalone/mw_main.lua
+++ b/engines/LuaStandalone/mw_main.lua
@@ -3,6 +3,6 @@
require('MWServer')
require('mw')
-server = MWServer:new()
+server = MWServer:new( arg[2] )
server:execute()
diff --git a/engines/LuaStandalone/protocol.txt
b/engines/LuaStandalone/protocol.txt
index a4e9915..7f75270 100644
--- a/engines/LuaStandalone/protocol.txt
+++ b/engines/LuaStandalone/protocol.txt
@@ -119,6 +119,20 @@
* nvalues: 0
* values: An empty array
+=== cleanupChunks ===
+
+Tell Lua to release any chunks no longer referenced by PHP.
+
+Message parameters:
+* op: "cleanupChunks"
+* ids: Table with keys being the chunk IDs still referenced by PHP, and
non-falsey values
+
+The response message is:
+
+* op: "return"
+* nvalues: 0
+* values: An empty array
+
=== quit ===
Request graceful shutdown.
diff --git a/tests/engines/LuaStandalone/LuaStandaloneInterpreterTest.php
b/tests/engines/LuaStandalone/LuaStandaloneInterpreterTest.php
index 7c40a43..0f1a4d3 100644
--- a/tests/engines/LuaStandalone/LuaStandaloneInterpreterTest.php
+++ b/tests/engines/LuaStandalone/LuaStandaloneInterpreterTest.php
@@ -47,4 +47,81 @@
$this->assertLessThan( 1.5, $time, 'getStatus() time usage' );
$this->assertEquals( $vsize, $status['vsize'], 'vsize', $vsize
* 0.1 );
}
+
+ function testFreeFunctions() {
+ $interpreter = $this->newInterpreter();
+
+ // Test #1: Make sure freeing actually works
+ $ret = $interpreter->callFunction(
+ $interpreter->loadString( 'return function() return
"testFreeFunction #1" end', 'test' )
+ );
+ $id = $ret[0]->id;
+ $interpreter->cleanupLuaChunks();
+ $this->assertEquals(
+ array( 'testFreeFunction #1' ),
$interpreter->callFunction( $ret[0] ),
+ 'Test that function #1 was not freed while a reference
exists'
+ );
+ $ret = null;
+ $interpreter->cleanupLuaChunks();
+ $testfunc = new Scribunto_LuaStandaloneInterpreterFunction(
$interpreter->id, $id );
+ try {
+ $interpreter->callFunction( $testfunc );
+ $this->fail( "Expected exception because function #1
should have been freed" );
+ } catch ( Scribunto_LuaError $e ) {
+ $this->assertEquals(
+ "Lua error: function id $id does not exist.",
$e->getMessage(),
+ 'Testing for expected error when calling a
freed function #1'
+ );
+ }
+
+ // Test #2: Make sure constructing a new copy of the function
works
+ $ret = $interpreter->callFunction(
+ $interpreter->loadString( 'return function() return
"testFreeFunction #2" end', 'test' )
+ );
+ $id = $ret[0]->id;
+ $func = new Scribunto_LuaStandaloneInterpreterFunction(
$interpreter->id, $id );
+ $ret = null;
+ $interpreter->cleanupLuaChunks();
+ $this->assertEquals(
+ array( 'testFreeFunction #2' ),
$interpreter->callFunction( $func ),
+ 'Test that function #2 was not freed while a reference
exists'
+ );
+ $func = null;
+ $interpreter->cleanupLuaChunks();
+ $testfunc = new Scribunto_LuaStandaloneInterpreterFunction(
$interpreter->id, $id );
+ try {
+ $interpreter->callFunction( $testfunc );
+ $this->fail( "Expected exception because function #2
should have been freed" );
+ } catch ( Scribunto_LuaError $e ) {
+ $this->assertEquals(
+ "Lua error: function id $id does not exist.",
$e->getMessage(),
+ 'Testing for expected error when calling a
freed function #2'
+ );
+ }
+
+ // Test #3: Make sure cloning works
+ $ret = $interpreter->callFunction(
+ $interpreter->loadString( 'return function() return
"testFreeFunction #3" end', 'test' )
+ );
+ $id = $ret[0]->id;
+ $func = clone $ret[0];
+ $ret = null;
+ $interpreter->cleanupLuaChunks();
+ $this->assertEquals(
+ array( 'testFreeFunction #3' ),
$interpreter->callFunction( $func ),
+ 'Test that function #3 was not freed while a reference
exists'
+ );
+ $func = null;
+ $interpreter->cleanupLuaChunks();
+ $testfunc = new Scribunto_LuaStandaloneInterpreterFunction(
$interpreter->id, $id );
+ try {
+ $interpreter->callFunction( $testfunc );
+ $this->fail( "Expected exception because function #3
should have been freed" );
+ } catch ( Scribunto_LuaError $e ) {
+ $this->assertEquals(
+ "Lua error: function id $id does not exist.",
$e->getMessage(),
+ 'Testing for expected error when calling a
freed function #3'
+ );
+ }
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/93073
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f15841051f7748d1d6df24080949e5cbd88f217
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Scribunto
Gerrit-Branch: master
Gerrit-Owner: Anomie <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
